Re: [PATCH v3] vt: keyboard: avoid signed integer overflow in k_ascii

[Dmitry Torokhov]

[ 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 <kt0755@xxxxxxxxx>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
> 
> - 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