[PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[Wlodzimierz Lipert]

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(-)

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
-- 
2.39.2

Re: [PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[Greg KH]

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

Re: [PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[Alan Stern]

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

Re: [PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[Greg KH]

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

Re: [PATCH v4] usb: prevent duplicate bEndpointAddress by usb_ep_autoconfig_ss (bitmap).

[Alan Stern]

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