divide error in usbtmc_generic_read

Linux-kernel-mentees jaskaransingh7654321 at gmail.com (Jaskaran Singh) 1 participant 5 messages Oct 3, 2019 – Oct 4, 2019

lore

Share & tools

lore

jaskaransingh7654321 at gmail.com (Jaskaran Singh)

2019-10-03 19:30 UTC #0 lore raw

Hi,

Following is my analysis of syzbot report:
https://syzkaller.appspot.com/bug?id=688d05e810900459b03c82fb48dc609edbbf7df9

The bug pertains to a divide error. The report's call trace is as follows:

 usbtmc_ioctl_generic_read drivers/usb/class/usbtmc.c:1029 [inline]
 usbtmc_ioctl+0x27d/0x2ab0 drivers/usb/class/usbtmc.c:2089
 vfs_ioctl fs/ioctl.c:46 [inline]
 file_ioctl fs/ioctl.c:509 [inline]
 do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
 ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
 __do_sys_ioctl fs/ioctl.c:720 [inline]
 __se_sys_ioctl fs/ioctl.c:718 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

The file:line where the error occurs is this:

	drivers/usb/class/usbtmc.c:816

Line 816 of that file is as follows:

		if ((max_transfer_size % data->wMaxPacketSize) == 0)

Running a 'git blame' on the file in question reveals that the last modification to line 816 was commit bb99794a4792068cb4bfd40e99e0f9d8fe7872fa. The commit adds the USBTMC_IOCTL_READ call for the driver, and thus the usbtmc_generic_read function.

I was not able to reproduce the error using my native machine and the same gcc version used by syzkaller (gcc 9.0.0 20181231). However, the error probably occurs because data->wMaxPacketSize is zero.

An intuitive fix for this seems to be:

- if ((max_transfer_size % data->wMaxPacketSize) == 0)
+ if (data->wMaxPacketSize &&
+ (max_transfer_size % data->wMaxPacketSize) == 0)

Or perhaps introduce a guard condition above line 816 to raise an error.

Cheers,
Jaskaran.

gregkh at linuxfoundation.org (Greg KH)

2019-10-03 19:56 UTC #1 lore raw

Quoted 40 lines · Jaskaran Singh

Hi,
Following is my analysis of syzbot report:
https://syzkaller.appspot.com/bug?id=688d05e810900459b03c82fb48dc609edbbf7df9
The bug pertains to a divide error. The report's call trace is as follows:
 usbtmc_ioctl_generic_read drivers/usb/class/usbtmc.c:1029 [inline]
 usbtmc_ioctl+0x27d/0x2ab0 drivers/usb/class/usbtmc.c:2089
 vfs_ioctl fs/ioctl.c:46 [inline]
 file_ioctl fs/ioctl.c:509 [inline]
 do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
 ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
 __do_sys_ioctl fs/ioctl.c:720 [inline]
 __se_sys_ioctl fs/ioctl.c:718 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
The file:line where the error occurs is this:
	drivers/usb/class/usbtmc.c:816
Line 816 of that file is as follows:
		if ((max_transfer_size % data->wMaxPacketSize) == 0)
Running a 'git blame' on the file in question reveals that the last modification to line 816 was commit bb99794a4792068cb4bfd40e99e0f9d8fe7872fa. The commit adds the USBTMC_IOCTL_READ call for the driver, and thus the usbtmc_generic_read function.
I was not able to reproduce the error using my native machine and the same gcc version used by syzkaller (gcc 9.0.0 20181231). However, the error probably occurs because data->wMaxPacketSize is zero.
An intuitive fix for this seems to be:
(quoted diff) +2 -1
-		if ((max_transfer_size % data->wMaxPacketSize) == 0)
+		if (data->wMaxPacketSize &&
+		    (max_transfer_size % data->wMaxPacketSize) == 0)

Quoted 1 line

Or perhaps introduce a guard condition above line 816 to raise an error.

Yes, if wMaxPacketSize is 0, then the code should just return an error. Care to make up a patch for this and submit it to syzbot to see if it solves the problem or not?

thanks,

greg k-h

jaskaransingh7654321 at gmail.com (Jaskaran Singh)

2019-10-04 06:58 UTC #2 lore raw

Quoted 54 lines · Greg KH

Quoted 45 lines · Jaskaran Singh
Hi,
Following is my analysis of syzbot report:
https://syzkaller.appspot.com/bug?id=688d05e810900459b03c82fb48dc609edbbf7df9
The bug pertains to a divide error. The report's call trace is as follows:
 usbtmc_ioctl_generic_read drivers/usb/class/usbtmc.c:1029 [inline]
 usbtmc_ioctl+0x27d/0x2ab0 drivers/usb/class/usbtmc.c:2089
 vfs_ioctl fs/ioctl.c:46 [inline]
 file_ioctl fs/ioctl.c:509 [inline]
 do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
 ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
 __do_sys_ioctl fs/ioctl.c:720 [inline]
 __se_sys_ioctl fs/ioctl.c:718 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
The file:line where the error occurs is this:
	drivers/usb/class/usbtmc.c:816
Line 816 of that file is as follows:
		if ((max_transfer_size % data->wMaxPacketSize) == 0)
Running a 'git blame' on the file in question reveals that the last modification to line 816 was commit bb99794a4792068cb4bfd40e99e0f9d8fe7872fa.
The commit adds the USBTMC_IOCTL_READ call for the driver, and thus the
usbtmc_generic_read function.
I was not able to reproduce the error using my native machine and the
same gcc version used by syzkaller (gcc 9.0.0 20181231). However, the
error probably occurs because data->wMaxPacketSize is zero.
An intuitive fix for this seems to be:
(quoted diff) +2 -1
-		if ((max_transfer_size % data->wMaxPacketSize) == 0)
+		if (data->wMaxPacketSize &&
+		    (max_transfer_size % data->wMaxPacketSize) == 0)
Quoted 2 lines
Or perhaps introduce a guard condition above line 816 to raise an error.
Yes, if wMaxPacketSize is 0, then the code should just return an error.
Care to make up a patch for this and submit it to syzbot to see if it solves the problem or not?

Quoted 3 lines

thanks,
greg k-h

Hi Greg,

Quoted 1 line

From the following thread:

https://groups.google.com/forum/#!searchin/syzkaller-bugs/divide$20error%7Csort:date/syzkaller-bugs/RYfg7J46e4c/t2amCUU7DgAJ

It looks like a patch has already been submitted and a crash does not trigger with it. However, it might make sense to add more sanity checking. Should I submit the patch?

Cheers,
Jaskaran.

gregkh at linuxfoundation.org (Greg KH)

2019-10-04 07:21 UTC #3 lore raw

Quoted 61 lines · Jaskaran Singh

Quoted 54 lines · Greg KH
Quoted 45 lines · Jaskaran Singh
Hi,
Following is my analysis of syzbot report:
https://syzkaller.appspot.com/bug?id=688d05e810900459b03c82fb48dc609edbbf7df9
The bug pertains to a divide error. The report's call trace is as follows:
 usbtmc_ioctl_generic_read drivers/usb/class/usbtmc.c:1029 [inline]
 usbtmc_ioctl+0x27d/0x2ab0 drivers/usb/class/usbtmc.c:2089
 vfs_ioctl fs/ioctl.c:46 [inline]
 file_ioctl fs/ioctl.c:509 [inline]
 do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
 ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
 __do_sys_ioctl fs/ioctl.c:720 [inline]
 __se_sys_ioctl fs/ioctl.c:718 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
The file:line where the error occurs is this:
	drivers/usb/class/usbtmc.c:816
Line 816 of that file is as follows:
		if ((max_transfer_size % data->wMaxPacketSize) == 0)
Running a 'git blame' on the file in question reveals that the last modification to line 816 was commit bb99794a4792068cb4bfd40e99e0f9d8fe7872fa.
The commit adds the USBTMC_IOCTL_READ call for the driver, and thus the
usbtmc_generic_read function.
I was not able to reproduce the error using my native machine and the
same gcc version used by syzkaller (gcc 9.0.0 20181231). However, the
error probably occurs because data->wMaxPacketSize is zero.
An intuitive fix for this seems to be:
(quoted diff) +2 -1
-		if ((max_transfer_size % data->wMaxPacketSize) == 0)
+		if (data->wMaxPacketSize &&
+		    (max_transfer_size % data->wMaxPacketSize) == 0)
Quoted 2 lines
Or perhaps introduce a guard condition above line 816 to raise an error.
Yes, if wMaxPacketSize is 0, then the code should just return an error.
Care to make up a patch for this and submit it to syzbot to see if it solves the problem or not?
Quoted 3 lines
thanks,
greg k-h
Hi Greg,

Quoted 7 lines

Quoted 1 line
From the following thread:
https://groups.google.com/forum/#!searchin/syzkaller-bugs/divide$20error%7Csort:date/syzkaller-bugs/RYfg7J46e4c/t2amCUU7DgAJ
It looks like a patch has already been submitted and a crash does not trigger with it. However, it might make sense to add more sanity checking. Should I submit the patch?

What more error checking can you add that is not already present with the patch that was submitted and accepted already upstream?

And why look at a report that is already fixed? Or is this one not properly closed as fixed? If not, can you please mark it as fixed?

thanks,

greg k-h

jaskaransingh7654321 at gmail.com (Jaskaran Singh)

2019-10-04 07:53 UTC #4 lore raw

Quoted 82 lines · Greg KH

Quoted 72 lines · Jaskaran Singh
Quoted 65 lines · Greg KH
Quoted 54 lines · Jaskaran Singh
Hi,
Following is my analysis of syzbot report:
https://syzkaller.appspot.com/bug?id=688d05e810900459b03c82fb48dc609edbbf7df9
The bug pertains to a divide error. The report's call trace is as
follows:
usbtmc_ioctl_generic_read drivers/usb/class/usbtmc.c:1029
[inline]
 usbtmc_ioctl+0x27d/0x2ab0 drivers/usb/class/usbtmc.c:2089
 vfs_ioctl fs/ioctl.c:46 [inline]
 file_ioctl fs/ioctl.c:509 [inline]
 do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
 ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
 __do_sys_ioctl fs/ioctl.c:720 [inline]
 __se_sys_ioctl fs/ioctl.c:718 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
The file:line where the error occurs is this:
	drivers/usb/class/usbtmc.c:816
Line 816 of that file is as follows:
		if ((max_transfer_size % data->wMaxPacketSize)
== 0)
Running a 'git blame' on the file in question reveals that the last
modification to line 816 was commit bb99794a4792068cb4bfd40e99e0f9d8fe7872fa.
The commit adds the USBTMC_IOCTL_READ call for the driver, and thus
the
usbtmc_generic_read function.
I was not able to reproduce the error using my native machine and
the
same gcc version used by syzkaller (gcc 9.0.0 20181231). However,
the
error probably occurs because data->wMaxPacketSize is zero.
An intuitive fix for this seems to be:
(quoted diff) +0 -1
-		if ((max_transfer_size % data->wMaxPacketSize)
== 0)
(quoted diff) +2 -0
+		if (data->wMaxPacketSize &&
+		    (max_transfer_size % data->wMaxPacketSize)
== 0)
Quoted 3 lines
Or perhaps introduce a guard condition above line 816 to raise an
error.
Yes, if wMaxPacketSize is 0, then the code should just return an error.
Care to make up a patch for this and submit it to syzbot to see if it
solves the problem or not?
Quoted 3 lines
thanks,
greg k-h
Hi Greg,
Quoted 8 lines
Quoted 1 line
From the following thread:
https://groups.google.com/forum/#!searchin/syzkaller-bugs/divide$20error%7Csort:date/syzkaller-bugs/RYfg7J46e4c/t2amCUU7DgAJ
It looks like a patch has already been submitted and a crash does not
trigger with it. However, it might make sense to add more sanity checking. Should I submit the patch?

Quoted 5 lines

What more error checking can you add that is not already present with the patch that was submitted and accepted already upstream?
And why look at a report that is already fixed? Or is this one not properly closed as fixed? If not, can you please mark it as fixed?

It wasn't closed or marked as fixed on the syzkaller portal. I'll be more careful and check the mailing list as well next time.

Cheers,
Jaskaran.

Quoted 3 lines

thanks,
greg k-h