> -----Original Message----- > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > Sent: Friday, May 22, 2020 3:24 AM > To: Changming Liu <liu.changm@xxxxxxxxxxxxxxxx> > Cc: thomas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value > > On Thu, May 21, 2020 at 05:56:44PM +0000, Changming Liu wrote: > > > > > > > -----Original Message----- > > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > > > Sent: Thursday, May 21, 2020 3:36 AM > > > To: Changming Liu <liu.changm@xxxxxxxxxxxxxxxx> > > > Cc: thomas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative > value > > > > > > On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote: > > > > 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 > > > > > > > > Signed-off-by: Changming Liu <liu.changm@xxxxxxxxxxxxxxxx> > > > > --- > > > > drivers/usb/misc/sisusbvga/sisusb.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c > > > b/drivers/usb/misc/sisusbvga/sisusb.c > > > > index fc8a5da4a07c..0734e6dd9386 100644 > > > > --- a/drivers/usb/misc/sisusbvga/sisusb.c > > > > +++ b/drivers/usb/misc/sisusbvga/sisusb.c > > > > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct > > > sisusb_usb_data *sisusb, u32 addr, > > > > u8 swap8, fromkern = kernbuffer ? 1 : 0; > > > > u16 swap16; > > > > u32 swap32, flag = (length >> 28) & 1; > > > > - char buf[4]; > > > > + u8 buf[4]; > > > > > > Do we also need to change the kernbuffer variable from char* to be u8* > > > as the same time to solve the same potential issue? > > > > > > > This is a very good point, sorry I didn't notice this. > > Indeed, according to the caller of sisusb_copy_memory, the wrapper of > current function > > there is no guarantee that each char in kernbuffer is positive. > > > > However, it seems if we change the function argument type directly from > char* to u8*, > > Other parts that call this function e.g. in sisusb_copy_memory > > or uses this pointer e.g. line 770,line 883 must change accordingly. > > Looks like many force casts which doesn't look too necessary. > > > > I wonder how about just force casting the content of kernbuffer when it's > read in line 823 to line 829 > > from char to u8? This seems explicitly fix this bug. > > That will work, but how about just changing all instances of char to u8 > throughout this driver to make sure everything is working properly that > way. char should not be used as a type when copying around "raw" data > like this from user-to-device for these reasons. > This is a clean sweep, from the perspective of security I find no reason against it. Indeed, u8 is strictly better than char when there is no need for any value to be negative. I'd be very honored to see this through. I wonder, by this driver, you mean this sisusbvga module or something else? Forgive me for my limited understanding of the module since I've only read the code related to this bug. Please let me know on what files do you want to apply this change. Or if you feel like doing this yourself please go ahead, I'm still a bit daunted by the scale of changes that need to be made frankly :p Best, Changming Liu
