[Bug Report] drivers/usb/misc/sisusbvga: integer overflow in sisusb_getidxreg and others

[Bug Report] drivers/usb/misc/sisusbvga: integer overflow in sisusb_getidxreg and others

[Changming Liu]

Hi Thomas,
Greetings, I'm a first-year PhD student who is interested in using UBSan for linux kernel. With some experiments, I found that in
drivers/usb/misc/sisusbvga/sisusb.c sisusb_getidxreg, there is an signed integer overflow which might cause unexpected result.

More specifically, starting from the fetch function in func sisusb_ioctl, line 2959, struct sisusb_command y is filled with data from user space. Then diving into 
sisusb_handle_command, the signed integer, named port, is casted from y->data3.
Then when executing sisusb_getidxreg, the signed integer, port, is used as 32-bit unsigned address in function sisusb_write_memio_byte.

The suspected problem is that, in sisusb_getidxreg, or other functions that have similar behavior, e.g. sisusb_setidxreg, the port integer value is directly from user space, and is used as port and port + 1. So port + 1 might overflow in a contrived way. With port being a signed integer, the overflown value might be undefined. Perhaps change the port integer from signed to unsigned would help?

To change the subject a little bit, I noted that, although all facing user input in sisusb_handle_command, sisusb_clear_vram does a good job vetting the address and length input from user to keep them in range, while the other functions like sisusb_setidxreg mentioned above, used the address without checking. I'm wondering, what caused the difference? Since this kind of check can eliminate the possibility of the overflow entirely.

Due to the lack of knowledge of the interactions between this driver and the user space and the real hardware, I'm not able to assess if this is an issue worth being dealt with, I'd be more than happy to hear your valuable opinions, this would help with understand UB and linux kernel a lot!

Looking forward to your valuable response!

Changming Liu

Re: [Bug Report] drivers/usb/misc/sisusbvga: integer overflow in sisusb_getidxreg and others

[Greg KH]

On Sun, Apr 19, 2020 at 10:04:06PM +0000, Changming Liu wrote:
> Hi Thomas,
> Greetings, I'm a first-year PhD student who is interested in using UBSan for linux kernel. With some experiments, I found that in
> drivers/usb/misc/sisusbvga/sisusb.c sisusb_getidxreg, there is an signed integer overflow which might cause unexpected result.
> 
> More specifically, starting from the fetch function in func sisusb_ioctl, line 2959, struct sisusb_command y is filled with data from user space. Then diving into 
> sisusb_handle_command, the signed integer, named port, is casted from y->data3.
> Then when executing sisusb_getidxreg, the signed integer, port, is used as 32-bit unsigned address in function sisusb_write_memio_byte.

Great, can you provide a patch fixing this so we can give you the proper
credit for finding and fixing the issue?

thanks,

greg k-h