USB: sisusbvga: Fix left shifting a possible negative value

Commit Message

Changming Liu May 20, 2020, 4:37 p.m. UTC

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Wednesday, May 20, 2020 1:02 AM
> To: Changming Liu <[e-mail]>
> Cc: [e-mail]; [e-mail]; Lu, Long
> <[e-mail]>; [e-mail]
> 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 <[e-mail]>
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 <[e-mail]>
---
 drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.17.1

> thanks,
> 
> greg k-h

Comments

Greg KH May 20, 2020, 4:47 p.m. UTC | #1

On Wed, May 20, 2020 at 04:37:50PM +0000, Changming Liu wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <[e-mail]>
> > Sent: Wednesday, May 20, 2020 1:02 AM
> > To: Changming Liu <[e-mail]>
> > Cc: [e-mail]; [e-mail]; Lu, Long
> > <[e-mail]>; [e-mail]
> > 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 <[e-mail]>
> 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

Changming Liu May 20, 2020, 6:14 p.m. UTC | #2

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Wednesday, May 20, 2020 12:47 PM
> To: Changming Liu <[e-mail]>
> Cc: [e-mail]; [e-mail]; Lu, Long
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> On Wed, May 20, 2020 at 04:37:50PM +0000, Changming Liu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH <[e-mail]>
> > > Sent: Wednesday, May 20, 2020 1:02 AM
> > > To: Changming Liu <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; Lu, Long
> > > <[e-mail]>; [e-mail]
> > > 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.
Sure, I'm so sorry for this inconvenience, I've sent you a separate patch , hope this works.
> 
> But:
> 
> >
> > Best,
> > Changming
> >
> >
> > >From 14ae7c67ea3fb96ed6bea0bc9919f3c597308813 Mon Sep 17 00:00:00
> > >2001
> > From: Changming Liu <[e-mail]>
> > 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.
[Changming Liu] 
Got it, I believe in my patch, I properly line-wrapped the line, I hope the patch you received is well-formatted. 
Otherwise, I'll have to send through my gmail account.

Sorry again for this inconvenience.

Best,
Changming

Patch

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