Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Alan Stern]

On Mon, Apr 03, 2023 at 10:54:05AM +0200, Oliver Neukum wrote:
> 
> 
> On 30.03.23 17:34, Alan Stern wrote:
> > Reference: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79
> > 
> > The sisusbvga driver just assumes that the endpoints it uses will be
> > present, without checking.  I don't know anything about this driver, so
> > the fix below may not be entirely correct.
> 
> Hi,
> 
> this patch by itself looks good to me.
> 
> But the need for it is problematic. Do we have any vendor specific driver
> that could get away without an equivalent to this patch without showing
> an equivalent bug?

Probably not.  Which is why adding this checking infrastructure to the 
USB core seems like a good idea, even though implementing it in each of 
the vendor-specific drivers may take quite a while.

>  If so, why do we have a generic matching code, although
> it is always insufficient?

(I assume you're referring to usb_match_device() and related routines.)

Not sufficient, perhaps, but necessary.  That is, in addition to 
checking the available endpoints, we also have to make sure the device 
has the right vendor ID, product ID, and so on to match the driver.

> What is the purpose of a generic binding interface in sysfs if every probe()
> method blocks it? Allowing a generic probe looks like a misdesign under these
> circumstances. You'd really want to add IDs to drivers.

I really don't understand what you're asking.  If you're talking about 
the "bind" and "unbind" files in /sys/bus/*/drivers/*/, they are there 
to allow manual binding and unbinding of devices.  Even though only one 
driver is likely to bind to any particular device.  (Furthermore, all 
this was true even before we started being careful about checking 
endpoints numbers and types.)

And we _do_ have IDs in drivers; that's what the .id_table member of 
struct usb_driver is for.

Alan Stern