Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Greg KH <greg@kroah.com>]

On Wed, May 20, 2020 at 04:37:50PM +0000, Changming Liu wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <greg@kroah.com>
> > Sent: Wednesday, May 20, 2020 1:02 AM
> > To: Changming Liu <liu.changm@northeastern.edu>
> > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org; Lu, Long
> > <l.lu@northeastern.edu>; yaohway@gmail.com
> > Subject: Re: [Bug Report] drivers/usb/misc/sisusbvga: undefined result when
> > left shift a possible negative value in sisusb_write_mem_bulk
> > 
> > On Wed, May 20, 2020 at 03:51:04AM +0000, Changming Liu wrote:
> > > Hi Greg and Thomas,
> > > Greetings, I'm a first-year PhD student who is interested in the usage of
> > UBSan for linux. And after some experiments, I've found that in
> > drivers/usb/misc/sisusbvga/sisusb.c
> > > function sisusb_write_mem_bulk, there is an undefined behavior caused by
> > left shifting a possible negative number.
> > >
> > > More specifically, in the switch statement for case 3, after executing
> > copy_from_user, the the lower 3 bytes of char buf[4] are filled with data from
> > user space.
> > > And these 3 bytes are left shifted accordingly to form a 32bit unsigned
> > integer, swap32.
> > >
> > > The potential problem is, since the buf is declared as signed char buffer so
> > each byte might be a negative number while being left shifted. According to the
> > C standard, when the left-hand operand of the left shift operator is a negative
> > value, the result is undefined. So I guess change the buf declaration to unsigned
> > will help? Given that it's only used here.
> > 
> > Sounds like a good idea, patches are welcome to fix this.
> 
> Hi greg,
> Thank you for this recognition! This means a lot to me. 
> Here's the patch as we agreed.

Please resend this in a normal format where we can properly review it.

But:

> 
> Best,
> Changming
> 
> 
> >From 14ae7c67ea3fb96ed6bea0bc9919f3c597308813 Mon Sep 17 00:00:00 2001
> From: Changming Liu <liu.changm@northeastern.edu>
> Date: Wed, 20 May 2020 12:19:37 -0400
> Subject: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> the char buffer buf, accepts user data which might be negative value and the content is left shifted to form an unsigned integer.
> Since left shifting a negative value is undefined behavior, thus change the char to u8 to fix this

Properly line-wrap your changelog when you resend this.

thanks,

greg k-h