On Sat, May 06, 2023 at 08:19:24AM -0400, Alan Stern wrote:
> On Sat, May 06, 2023 at 09:44:17AM +0900, Greg KH wrote:
> > On Sat, Apr 29, 2023 at 05:47:33PM +0200, Wlodzimierz Lipert wrote:
>
> > > --- a/drivers/usb/gadget/epautoconf.c
> > > +++ b/drivers/usb/gadget/epautoconf.c
> > > @@ -67,6 +67,8 @@ struct usb_ep *usb_ep_autoconfig_ss(
> > > )
> > > {
> > > struct usb_ep *ep;
> > > + unsigned *epmap;
> >
> > Why "unsigned"? Didn't checkpatch complain about this?
>
> In other words, the preferred style in the kernel is to use "unsigned
> int" rather than just "unsigned". (Although there's plenty of old code
> that still does it that way.)
>
> > > if (isdigit(ep->name[2])) {
> > > - u8 num = simple_strtoul(&ep->name[2], NULL, 10);
> > > - desc->bEndpointAddress |= num;
> > > - } else if (desc->bEndpointAddress & USB_DIR_IN) {
> > > - if (++gadget->in_epnum > 15)
> > > + num = simple_strtoul(&ep->name[2], NULL, 10);
> > > + WARN_ON(num == 0 || num > 15);
> >
> > You just crashed the system if this happened if panic-on-warn is enabled :(
> >
> > Please never do this, if there is an issue, fix it up and handle it
> > properly.
>
> I disagree; I think this is an appropriate use of WARN_ON. It's an
> example of "drivers should never do this, and if they do then weird,
> unpredictable errors will occur". For these things we _want_ to have a
> very visible notice that a problem needs to be fixed. (In this case it
> wouldn't hurt to also print out the bad endpoint name as well, along
> with the name of the UDC driver.)
>
> If someone turns on panic-on-warn, it means they _do_ want their system
> to crash when issues like this crop up. Like syzbot, for example. If
> instead we just covered up the error then it would never get fixed.
So you want syzbot to constantly be triggered here by sending in bad
addresses? If this is something that a user can trigger, it should
never have a WARN_ON(). Or is this only something that a badly written
driver can trigger? It's hard to tell from the snippet above.
thanks,
greg k-h
