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
