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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, May 25, 2020 at 09:15:07AM +0200, Greg KH wrote:
> 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?

I disagree, as this is simply an accumulator of an indeterminate size.
We are not talking to hardware here, so it does not have to be 32 bits,
just "large enough".

I'll resubmit with "unsigned int" and if you feel strongly about this
please tweak the patch to taste ;)

Thanks!

-- 
Dmitry