lkml.org 
[lkml]   [2020]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] vt: keyboard: avoid integer overflow in k_ascii
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

\
 
 \ /
  Last update: 2020-05-25 02:09    [W:0.044 / U:1.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site