[Linux-kernel-mentees] [SYZBOT REPORT] divide error in usbtmc_generic_read

[jaskaransingh7654321 at gmail.com (Jaskaran Singh)]

On Fri, 2019-10-04 at 09:21 +0200, Greg KH wrote:
> On Fri, Oct 04, 2019 at 12:28:32PM +0530, Jaskaran Singh wrote:
> > On Thu, 2019-10-03 at 21:56 +0200, Greg KH wrote:
> > > On Fri, Oct 04, 2019 at 01:00:16AM +0530, Jaskaran Singh wrote:
> > > > 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.
> > > 
> > > 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
> > 
> > Hi Greg,
> > 
> > > 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?

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.


> thanks,
> 
> greg k-h