> -----Original Message-----
> From: Greg KH <greg@xxxxxxxxx>
> Sent: Wednesday, May 20, 2020 1:02 AM
> To: Changming Liu <liu.changm@xxxxxxxxxxxxxxxx>
> Cc: thomas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Lu, Long
> <l.lu@xxxxxxxxxxxxxxxx>; yaohway@xxxxxxxxx
> 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.
Best,
Changming
>From 14ae7c67ea3fb96ed6bea0bc9919f3c597308813 Mon Sep 17 00:00:00 2001
From: Changming Liu <liu.changm@xxxxxxxxxxxxxxxx>
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
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];
/* if neither kernbuffer not userbuffer are given, assume
* data in obuf
--
2.17.1
> thanks,
>
> greg k-h
