[PATCHv2] usb: usb_parse_endpoint ignore reserved bits

[PATCHv2] usb: usb_parse_endpoint ignore reserved bits

[Oliver Neukum]

Reading bEndpointAddress the spec tells is
that

b7 is direction, which must be ignored
b6:4 are reserved which are to be set to zero
b3:0 are the endpoint address

In order to be backwards compatible with possible
future versions of USB we have to be ready with
devices using those bits. That means that we
also have to ignore them like we do with the direction
bit.
In consequence the only illegal address you can
encoding in four bits is endpoint zero, for which
no descriptor must exist. Hence the check for exceeding
the upper limit on endpoint addresses is removed.

Signed-off-by: Oliver Neukum <oneukum@suse.com>

V2: Improved commit log
---
 drivers/usb/core/config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 8fd4208d17db..43c5ed256e6e 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -285,11 +285,11 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
-	i = d->bEndpointAddress & ~USB_ENDPOINT_DIR_MASK;
-	if (i >= 16 || i == 0) {
+	i = d->bEndpointAddress & 0x0f;
+	if (i == 0) {
 		dev_notice(ddev, "config %d interface %d altsetting %d has an "
-		    "invalid endpoint with address 0x%X, skipping\n",
-		    cfgno, inum, asnum, d->bEndpointAddress);
+		    "invalid descriptor for the common control endpoint, skipping\n",
+		    cfgno, inum, asnum);
 		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
-- 
2.44.0

Re: [PATCHv2] usb: usb_parse_endpoint ignore reserved bits

[Greg KH]

On Thu, Apr 18, 2024 at 01:02:21PM +0200, Oliver Neukum wrote:
> Reading bEndpointAddress the spec tells is
> that
> 
> b7 is direction, which must be ignored
> b6:4 are reserved which are to be set to zero
> b3:0 are the endpoint address
> 
> In order to be backwards compatible with possible
> future versions of USB we have to be ready with
> devices using those bits. That means that we
> also have to ignore them like we do with the direction
> bit.
> In consequence the only illegal address you can
> encoding in four bits is endpoint zero, for which
> no descriptor must exist. Hence the check for exceeding
> the upper limit on endpoint addresses is removed.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> 
> V2: Improved commit log

Nit, "V2" goes below the --- line.

> ---
>  drivers/usb/core/config.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 8fd4208d17db..43c5ed256e6e 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -285,11 +285,11 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  		goto skip_to_next_endpoint_or_interface_descriptor;
>  	}
>  
> -	i = d->bEndpointAddress & ~USB_ENDPOINT_DIR_MASK;
> -	if (i >= 16 || i == 0) {
> +	i = d->bEndpointAddress & 0x0f;

Using a #define here instead of 0x0f might be good, right?

thanks,

greg k-h

Re: [PATCHv2] usb: usb_parse_endpoint ignore reserved bits

[Alan Stern]

On Thu, Apr 18, 2024 at 01:02:21PM +0200, Oliver Neukum wrote:
> Reading bEndpointAddress the spec tells is
> that
> 
> b7 is direction, which must be ignored
> b6:4 are reserved which are to be set to zero
> b3:0 are the endpoint address
> 
> In order to be backwards compatible with possible
> future versions of USB we have to be ready with
> devices using those bits. That means that we
> also have to ignore them like we do with the direction
> bit.
> In consequence the only illegal address you can
> encoding in four bits is endpoint zero, for which
> no descriptor must exist. Hence the check for exceeding
> the upper limit on endpoint addresses is removed.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> 
> V2: Improved commit log
> ---
>  drivers/usb/core/config.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 8fd4208d17db..43c5ed256e6e 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -285,11 +285,11 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  		goto skip_to_next_endpoint_or_interface_descriptor;
>  	}
>  
> -	i = d->bEndpointAddress & ~USB_ENDPOINT_DIR_MASK;
> -	if (i >= 16 || i == 0) {
> +	i = d->bEndpointAddress & 0x0f;

See my earlier reply.  The 0x0f here should be USB_ENDPOINT_NUMBER_MASK.  
Or even better, you could use the usb_endpoint_num() inline routine.

> +	if (i == 0) {
>  		dev_notice(ddev, "config %d interface %d altsetting %d has an "
> -		    "invalid endpoint with address 0x%X, skipping\n",
> -		    cfgno, inum, asnum, d->bEndpointAddress);
> +		    "invalid descriptor for the common control endpoint, skipping\n",

"endpoint 0" would be simpler than "the common control endpoint".

Alan Stern

> +		    cfgno, inum, asnum);
>  		goto skip_to_next_endpoint_or_interface_descriptor;
>  	}
>  
> -- 
> 2.44.0
>