linux-kernel - Re: [PATCH v2] vt: keyboard: avoid integer overflow in k

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 25 May 2020 15:51:57 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Kyungtae Kim <kt0755@...il.com>, Jiri Slaby <jslaby@...e.com>,
        syzkaller <syzkaller@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Dave Tian <dave.jing.tian@...il.com>
Subject: Re: [PATCH v2] vt: keyboard: avoid integer overflow in k_ascii

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