[ Sorry, forgot to CC Kyungtae ]
On Mon, May 25, 2020 at 04:27:40PM -0700, Dmitry Torokhov wrote:
> When k_ascii is invoked several times in a row there is a potential for
> signed integer overflow:
>
> UBSAN: Undefined behaviour in drivers/tty/vt/keyboard.c:888:19 signed integer
> overflow:
> 10 * 1111111111 cannot be represented in type 'int'
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.11 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xce/0x128 lib/dump_stack.c:118
> ubsan_epilogue+0xe/0x30 lib/ubsan.c:154
> handle_overflow+0xdc/0xf0 lib/ubsan.c:184
> __ubsan_handle_mul_overflow+0x2a/0x40 lib/ubsan.c:205
> k_ascii+0xbf/0xd0 drivers/tty/vt/keyboard.c:888
> kbd_keycode drivers/tty/vt/keyboard.c:1477 [inline]
> kbd_event+0x888/0x3be0 drivers/tty/vt/keyboard.c:1495
>
> While it can be worked around by using check_mul_overflow()/
> check_add_overflow(), it is better to introduce a separate flag to
> signal that number pad is being used to compose a symbol, and
> change type of the accumulator from signed to unsigned, thus
> avoiding undefined behavior when it overflows.
>
> Reported-by: Kyungtae Kim <kt0...@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
> ---
>
> - marking the patch as v3 as it is a successor of Kyungtae's patches.
>
> drivers/tty/vt/keyboard.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 15d33fa0c925..568b2171f335 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -127,7 +127,11 @@ static DEFINE_SPINLOCK(func_buf_lock); /* guard
> 'func_buf' and friends */
> static unsigned long key_down[BITS_TO_LONGS(KEY_CNT)]; /* keyboard key
> bitmap */
> static unsigned char shift_down[NR_SHIFT]; /* shift state
> counters.. */
> static bool dead_key_next;
> -static int npadch = -1; /* -1 or number
> assembled on pad */
> +
> +/* Handles a number being assembled on the number pad */
> +static bool npadch_active;
> +static unsigned int npadch_value;
> +
> static unsigned int diacr;
> static char rep; /* flag telling
> character repeat */
>
> @@ -845,12 +849,12 @@ static void k_shift(struct vc_data *vc, unsigned char
> value, char up_flag)
> shift_state &= ~(1 << value);
>
> /* kludge */
> - if (up_flag && shift_state != old_state && npadch != -1) {
> + if (up_flag && shift_state != old_state && npadch_active) {
> if (kbd->kbdmode == VC_UNICODE)
> - to_utf8(vc, npadch);
> + to_utf8(vc, npadch_value);
> else
> - put_queue(vc, npadch & 0xff);
> - npadch = -1;
> + put_queue(vc, npadch_value & 0xff);
> + npadch_active = false;
> }
> }
>
> @@ -868,7 +872,7 @@ static void k_meta(struct vc_data *vc, unsigned char
> value, char up_flag)
>
> static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
> {
> - int base;
> + unsigned int base;
>
> if (up_flag)
> return;
> @@ -882,10 +886,12 @@ static void k_ascii(struct vc_data *vc, unsigned char
> value, char up_flag)
> base = 16;
> }
>
> - if (npadch == -1)
> - npadch = value;
> - else
> - npadch = npadch * base + value;
> + if (!npadch_active) {
> + npadch_value = 0;
> + npadch_active = true;
> + }
> +
> + npadch_value = npadch_value * base + value;
> }
>
> static void k_lock(struct vc_data *vc, unsigned char value, char up_flag)
> --
> 2.27.0.rc0.183.gde8f92d652-goog
>
>
> --
> Dmitry
--
Dmitry
