On Sat, May 06, 2023 at 02:29:49PM +0200, Greg KH wrote:
> 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:
> > > > 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?
I don't know about the "constantly" part, but otherwise, yes.
> 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.
Only a badly written driver. Or possibly (I'm not sure) a bad user of
gadgetfs or functionfs -- but those things require superuser permission.
Alan Stern
