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

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

[Dmitry Torokhov]

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


I think if we stop overloading what npadch means, the code becomes more
clear. What do you think?

Thanks.

-- 
Dmitry

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

[Greg KH]

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

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

[Dmitry Torokhov]

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