| Message ID | 20230429154733.4429-1-wlodzimierz.lipert@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap). | expand |
On Sat, Apr 29, 2023 at 05:47:33PM +0200, Wlodzimierz Lipert wrote: > usb_ep_autoconfig_ss tries to use endpoint name or internal counters to generate > bEndpointAddress - this leads to duplicate addresses. Fix changes the > way in/out_epnum is used, now as bitmap which represents unavailable ep numbers. Please properly wrap your changelog lines at 72 columns like your editor asked you to. > v2: formatting fixes only (errors). > v3: refined autoconf logic (removed num_mask, switched to use ffz). > v4: formatting only changes (style). As per the documentation, this information about changes needs to go below the --- line. > > Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com> > --- > drivers/usb/gadget/epautoconf.c | 25 ++++++++++++++++--------- > include/linux/usb/gadget.h | 4 ++-- > 2 files changed, 18 insertions(+), 11 deletions(-) What commit id does this fix? Does it need to go to older stable kernels? If so, how far back? > > diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c > index 1eb4fa2e623f..f73a7fe3d7d7 100644 > --- 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? > + u8 num; > > if (gadget->ops->match_ep) { > ep = gadget->ops->match_ep(gadget, desc, ep_comp); > @@ -93,19 +95,23 @@ struct usb_ep *usb_ep_autoconfig_ss( > > /* report address */ > desc->bEndpointAddress &= USB_DIR_IN; > + epmap = usb_endpoint_dir_in(desc) ? > + &gadget->in_epnum : &gadget->out_epnum; I hate ?: lines with a passion. Please spell it out with a real if statement as the generated code is the same and it's easier to actually read by humans. > + > 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. > + if (*epmap & (1U << num)) > return NULL; > - desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum; > } else { > - if (++gadget->out_epnum > 15) > + /* find first available ep number (not encoded in ep name) */ > + num = ffz(*epmap); > + if (num > 15) > return NULL; > - desc->bEndpointAddress |= gadget->out_epnum; > } > > + *epmap |= 1U << num; BIT()? > + desc->bEndpointAddress |= num; > ep->address = desc->bEndpointAddress; > ep->desc = NULL; > ep->comp_desc = NULL; > @@ -208,7 +214,8 @@ void usb_ep_autoconfig_reset (struct usb_gadget *gadget) > ep->claimed = false; > ep->driver_data = NULL; > } > - gadget->in_epnum = 0; > - gadget->out_epnum = 0; > + /* ep num 0 is reserved: not available for auto configuration */ > + gadget->in_epnum = 1U; > + gadget->out_epnum = 1U; > } > EXPORT_SYMBOL_GPL(usb_ep_autoconfig_reset); > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 6a178177e4c9..1e00e22202bc 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -343,8 +343,8 @@ struct usb_gadget_ops { > * and sometimes configuration. > * @dev: Driver model state for this abstract device. > * @isoch_delay: value from Set Isoch Delay request. Only valid on SS/SSP > - * @out_epnum: last used out ep number > - * @in_epnum: last used in ep number > + * @out_epnum: bitmap of allocated out ep numbers > + * @in_epnum: bitmap of allocated in ep numbers As you changed the functionality of these variables, why not rename them at the same time to make it obvious? 'in_epnum_bitmap'? thanks, greg k-h
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. Alan Stern
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
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
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c index 1eb4fa2e623f..f73a7fe3d7d7 100644 --- 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; + u8 num; if (gadget->ops->match_ep) { ep = gadget->ops->match_ep(gadget, desc, ep_comp); @@ -93,19 +95,23 @@ struct usb_ep *usb_ep_autoconfig_ss( /* report address */ desc->bEndpointAddress &= USB_DIR_IN; + epmap = usb_endpoint_dir_in(desc) ? + &gadget->in_epnum : &gadget->out_epnum; + 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); + if (*epmap & (1U << num)) return NULL; - desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum; } else { - if (++gadget->out_epnum > 15) + /* find first available ep number (not encoded in ep name) */ + num = ffz(*epmap); + if (num > 15) return NULL; - desc->bEndpointAddress |= gadget->out_epnum; } + *epmap |= 1U << num; + desc->bEndpointAddress |= num; ep->address = desc->bEndpointAddress; ep->desc = NULL; ep->comp_desc = NULL; @@ -208,7 +214,8 @@ void usb_ep_autoconfig_reset (struct usb_gadget *gadget) ep->claimed = false; ep->driver_data = NULL; } - gadget->in_epnum = 0; - gadget->out_epnum = 0; + /* ep num 0 is reserved: not available for auto configuration */ + gadget->in_epnum = 1U; + gadget->out_epnum = 1U; } EXPORT_SYMBOL_GPL(usb_ep_autoconfig_reset); diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 6a178177e4c9..1e00e22202bc 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -343,8 +343,8 @@ struct usb_gadget_ops { * and sometimes configuration. * @dev: Driver model state for this abstract device. * @isoch_delay: value from Set Isoch Delay request. Only valid on SS/SSP - * @out_epnum: last used out ep number - * @in_epnum: last used in ep number + * @out_epnum: bitmap of allocated out ep numbers + * @in_epnum: bitmap of allocated in ep numbers * @mA: last set mA value * @otg_caps: OTG capabilities of this gadget. * @sg_supported: true if we can handle scatter-gather
usb_ep_autoconfig_ss tries to use endpoint name or internal counters to generate bEndpointAddress - this leads to duplicate addresses. Fix changes the way in/out_epnum is used, now as bitmap which represents unavailable ep numbers. v2: formatting fixes only (errors). v3: refined autoconf logic (removed num_mask, switched to use ffz). v4: formatting only changes (style). Signed-off-by: Wlodzimierz Lipert <wlodzimierz.lipert@gmail.com> --- drivers/usb/gadget/epautoconf.c | 25 ++++++++++++++++--------- include/linux/usb/gadget.h | 4 ++-- 2 files changed, 18 insertions(+), 11 deletions(-)