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

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, May 24, 2020 at 05:08:23PM -0700, Dmitry Torokhov wrote:
> On Sat, May 23, 2020 at 11:09:35PM +0000, Kyungtae Kim wrote:
> > @@ -884,8 +884,11 @@ static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
> >  
> >  	if (npadch == -1)
> >  		npadch = value;
> > +	else if (!check_mul_overflow(npadch, base, &new_npadch) &&
> > +	    !check_add_overflow(new_npadch, value, &new_npadch))
> > +		npadch = new_npadch;
> >  	else
> > -		npadch = npadch * base + value;
> > +		return;
> >  }
> 
> So thinking about it some more, if we use unsigned types, then there is
> no issue with overflow UB, and thus maybe we should do something like
> this:
> 
> 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;

Much nicer, thanks for that, -1 is not a good thing to try to understand :)

> +static unsigned int npadch_value;

Nicer to just make this a u32 to be explicit about it?

> +
>  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;

u32?


>  
>  	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)
> 
> 
> I think if we stop overloading what npadch means, the code becomes more
> clear. What do you think?

I think it makes a lot more sense, care to turn this into a "real"
patch?

thanks,
greg k-h