[Bug Report] drivers/usb/misc/sisusbvga: undefined result when left shift a possible negative value in sisusb_write_mem_bulk

[Bug Report] drivers/usb/misc/sisusbvga: undefined result when left shift a possible negative value in sisusb_write_mem_bulk

[Changming Liu]

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.

Due to the lack of knowledge of the interaction between this module and others, I'm not able to assess the severity of this problem. I wonder if it's worth a fix? If not, I would appreciate it if I can know why, this will help me understand linux and UB a lot!

Looking forward to your valuable response!

PS: I'm that guy who sent you a bug report and the patch was accepted 3 weeks ago, please allow me to express my appreciation again!

Changming Liu

Re: [Bug Report] drivers/usb/misc/sisusbvga: undefined result when left shift a possible negative value in sisusb_write_mem_bulk

[Greg KH]

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.

thanks,

greg k-h