Hardening the parser during enumerations

Hardening the parser during enumerations

[Oliver Neukum]

The parser we use to enumerate a new device has no hardening
against nonsensical descriptors at all. This is a bit optimistic

[RFC 1/6] usb: usb_parse_endpoint ignore reserved bits

[Oliver Neukum]

We have to ignore the higher bits in bEndpointAddress

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 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 7f8d33f92ddb..c7056b123d46 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -279,11 +279,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: [RFC 1/6] usb: usb_parse_endpoint ignore reserved bits

[Greg KH]

On Thu, Apr 11, 2024 at 02:42:59PM +0200, Oliver Neukum wrote:
> We have to ignore the higher bits in bEndpointAddress

Why?

> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  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 7f8d33f92ddb..c7056b123d46 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -279,11 +279,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);

So now we just ignore invalid descriptors here and let them pass?

confused,

greg k-h

Re: [RFC 1/6] usb: usb_parse_endpoint ignore reserved bits

[Oliver Neukum]

On 11.04.24 16:11, Greg KH wrote:
> On Thu, Apr 11, 2024 at 02:42:59PM +0200, Oliver Neukum wrote:
>> We have to ignore the higher bits in bEndpointAddress
> 
> Why?

Because if we do not, we are breaking compatibility with all future
standards that use those bits in backwards compatible manner.

	Regards
		Oliver

Re: [RFC 1/6] usb: usb_parse_endpoint ignore reserved bits

[Greg KH]

On Thu, Apr 11, 2024 at 04:27:26PM +0200, Oliver Neukum wrote:
> On 11.04.24 16:11, Greg KH wrote:
> > On Thu, Apr 11, 2024 at 02:42:59PM +0200, Oliver Neukum wrote:
> > > We have to ignore the higher bits in bEndpointAddress
> > 
> > Why?
> 
> Because if we do not, we are breaking compatibility with all future
> standards that use those bits in backwards compatible manner.

Ok, that's great, then say that in the changelog please :)

thanks,

greg k-h

Re: [RFC 1/6] usb: usb_parse_endpoint ignore reserved bits

[Alan Stern]

On Thu, Apr 11, 2024 at 02:42:59PM +0200, Oliver Neukum wrote:
> We have to ignore the higher bits in bEndpointAddress

Mention that these bits are reserved.  That's why we ought to ignore 
them.

Also, this is not really an example of hardening; it's more like 
future-proofing the code.  The existing code will work fine with a 
malicious device; your real concern is about possible changes to the 
spec in the future.

> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  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 7f8d33f92ddb..c7056b123d46 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -279,11 +279,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;

Use USB_ENDPOINT_NUMBER_MASK, not 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",

It would be clearer to say "endpoint 0" instead of "the common control 
endpoint".  (The spec uses that phrase; it doesn't mean this is the best 
way of saying it.)

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

Otherwise this is okay.

Alan Stern

Re: [RFC 1/6] usb: usb_parse_endpoint ignore reserved bits

[Oliver Neukum]

On 11.04.24 17:35, Alan Stern wrote:
> On Thu, Apr 11, 2024 at 02:42:59PM +0200, Oliver Neukum wrote:
>> We have to ignore the higher bits in bEndpointAddress
> 
> Mention that these bits are reserved.  That's why we ought to ignore
> them.

OK

> Also, this is not really an example of hardening; it's more like
> future-proofing the code.  The existing code will work fine with a
> malicious device; your real concern is about possible changes to the
> spec in the future.

It seemed to me like a vector for a DoS, but in hindsight, yes I'll
take it out of the series.

	Regards
		Oliver

[RFC 2/6] usb: avoid overrunning a buffer in usb_parse_interface

[Oliver Neukum]

We must not touch bDescriptorType if it is not within our buffer.
To guarantee that we have to be sure the first two bytes of the
descriptor are within the buffer.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index c7056b123d46..5891652b6202 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -575,7 +575,7 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 
 	/* Parse all the endpoint descriptors */
 	n = 0;
-	while (size > 0) {
+	while (size >= sizeof(struct usb_descriptor_header)) { /* minimum length to get bDescriptorType */
 		if (((struct usb_descriptor_header *) buffer)->bDescriptorType
 		     == USB_DT_INTERFACE)
 			break;
-- 
2.44.0

Re: [RFC 2/6] usb: avoid overrunning a buffer in usb_parse_interface

[Alan Stern]

On Thu, Apr 11, 2024 at 02:43:00PM +0200, Oliver Neukum wrote:
> We must not touch bDescriptorType if it is not within our buffer.
> To guarantee that we have to be sure the first two bytes of the
> descriptor are within the buffer.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index c7056b123d46..5891652b6202 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -575,7 +575,7 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
>  
>  	/* Parse all the endpoint descriptors */
>  	n = 0;
> -	while (size > 0) {
> +	while (size >= sizeof(struct usb_descriptor_header)) { /* minimum length to get bDescriptorType */
>  		if (((struct usb_descriptor_header *) buffer)->bDescriptorType
>  		     == USB_DT_INTERFACE)
>  			break;

I would omit the comment (the point seems pretty obvious even though I 
never noticed it before), but there's nothing wrong with having it.

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [RFC 2/6] usb: avoid overrunning a buffer in usb_parse_interface

[Alan Stern]

On Thu, Apr 11, 2024 at 11:39:01AM -0400, Alan Stern wrote:
> On Thu, Apr 11, 2024 at 02:43:00PM +0200, Oliver Neukum wrote:
> > We must not touch bDescriptorType if it is not within our buffer.
> > To guarantee that we have to be sure the first two bytes of the
> > descriptor are within the buffer.
> > 
> > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > ---
> >  drivers/usb/core/config.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> > index c7056b123d46..5891652b6202 100644
> > --- a/drivers/usb/core/config.c
> > +++ b/drivers/usb/core/config.c
> > @@ -575,7 +575,7 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
> >  
> >  	/* Parse all the endpoint descriptors */
> >  	n = 0;
> > -	while (size > 0) {
> > +	while (size >= sizeof(struct usb_descriptor_header)) { /* minimum length to get bDescriptorType */
> >  		if (((struct usb_descriptor_header *) buffer)->bDescriptorType
> >  		     == USB_DT_INTERFACE)
> >  			break;
> 
> I would omit the comment (the point seems pretty obvious even though I 
> never noticed it before), but there's nothing wrong with having it.
> 
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

I take this back.  The checks performed by usb_parse_configuration() 
make this unnecessary.

Alan Stern

[RFC 3/6] usb: usb_parse_endpoint needs to guard against short descriptors

[Oliver Neukum]

If a malicious device gives us a descriptor of zero length
we'll go into an infinite loop. We have to check and do
a hard bailout.
If we get a descriptor of length < 2 we'll parse the next
descriptor as part of the current descriptor. We need to check.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/config.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 5891652b6202..050cd5066ccf 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -265,6 +265,9 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 	const unsigned short *maxpacket_maxes;
 
 	d = (struct usb_endpoint_descriptor *) buffer;
+	if (d->bLength < sizeof(struct usb_descriptor_header)) /* this amounts to sabotage */
+		return -EINVAL;
+
 	buffer += d->bLength;
 	size -= d->bLength;
 
-- 
2.44.0

Re: [RFC 3/6] usb: usb_parse_endpoint needs to guard against short descriptors

[Alan Stern]

On Thu, Apr 11, 2024 at 02:43:01PM +0200, Oliver Neukum wrote:
> If a malicious device gives us a descriptor of zero length
> we'll go into an infinite loop. We have to check and do
> a hard bailout.
> If we get a descriptor of length < 2 we'll parse the next
> descriptor as part of the current descriptor. We need to check.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/config.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 5891652b6202..050cd5066ccf 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -265,6 +265,9 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  	const unsigned short *maxpacket_maxes;
>  
>  	d = (struct usb_endpoint_descriptor *) buffer;
> +	if (d->bLength < sizeof(struct usb_descriptor_header)) /* this amounts to sabotage */
> +		return -EINVAL;

Your 6/6 patch should guarantee that this can never happen.  Then this 
check won't be needed.

Alan Stern

[RFC 4/6] usb: usb_parse_endpoint guard against an incromprehensible preamble

[Oliver Neukum]

usb_parse_endpoint processes an endpoint descriptor and then
advances the parser to the next endpoint descriptor.
However, a malicious device could feature something other than
an endpoint descriptor after the interface descriptor

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 050cd5066ccf..055910fc6b19 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -271,6 +271,8 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 	buffer += d->bLength;
 	size -= d->bLength;
 
+	if (d->bDescriptorType != USB_DT_ENDPOINT)
+		goto skip_to_next_endpoint_or_interface_descriptor;
 	if (d->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE)
 		n = USB_DT_ENDPOINT_AUDIO_SIZE;
 	else if (d->bLength >= USB_DT_ENDPOINT_SIZE)
-- 
2.44.0

Re: [RFC 4/6] usb: usb_parse_endpoint guard against an incromprehensible preamble

[Alan Stern]

On Thu, Apr 11, 2024 at 02:43:02PM +0200, Oliver Neukum wrote:
> usb_parse_endpoint processes an endpoint descriptor and then
> advances the parser to the next endpoint descriptor.
> However, a malicious device could feature something other than
> an endpoint descriptor after the interface descriptor
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/config.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 050cd5066ccf..055910fc6b19 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -271,6 +271,8 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  	buffer += d->bLength;
>  	size -= d->bLength;
>  
> +	if (d->bDescriptorType != USB_DT_ENDPOINT)
> +		goto skip_to_next_endpoint_or_interface_descriptor;
>  	if (d->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE)
>  		n = USB_DT_ENDPOINT_AUDIO_SIZE;
>  	else if (d->bLength >= USB_DT_ENDPOINT_SIZE)

Not needed, because usb_parse_interface() calls find_next_descriptor() 
to find an endpoint descriptor or another interface descriptor before 
the first time it calls usb_parse_endpoint().

Alan Stern

[RFC 5/6] usb: usb_parse_endpoint must not count duplicated endpoints

[Oliver Neukum]

When an interface is parsed the number of endpoints claimed to exist
is compared to the number of endpoint descriptors actually found.
Duplicated endpoints are not parsed in usb_parse_endpoint but
usb_parse_interface counts them. That makes no sense.
To correct this usb_parse_endpoint needs to return feedback
about the validity of parsed endpoints.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/config.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 055910fc6b19..50acc9021247 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -254,7 +254,7 @@ static bool config_endpoint_is_duplicate(struct usb_host_config *config,
 static int usb_parse_endpoint(struct device *ddev, int cfgno,
 		struct usb_host_config *config, int inum, int asnum,
 		struct usb_host_interface *ifp, int num_ep,
-		unsigned char *buffer, int size)
+		unsigned char *buffer, int size, bool *valid)
 {
 	struct usb_device *udev = to_usb_device(ddev);
 	unsigned char *buffer0 = buffer;
@@ -270,6 +270,7 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 
 	buffer += d->bLength;
 	size -= d->bLength;
+	*valid = false;
 
 	if (d->bDescriptorType != USB_DT_ENDPOINT)
 		goto skip_to_next_endpoint_or_interface_descriptor;
@@ -313,6 +314,7 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 		}
 	}
 
+	*valid = true;
 	endpoint = &ifp->endpoint[ifp->desc.bNumEndpoints];
 	++ifp->desc.bNumEndpoints;
 
@@ -581,14 +583,17 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 	/* Parse all the endpoint descriptors */
 	n = 0;
 	while (size >= sizeof(struct usb_descriptor_header)) { /* minimum length to get bDescriptorType */
+		bool valid;
+
 		if (((struct usb_descriptor_header *) buffer)->bDescriptorType
 		     == USB_DT_INTERFACE)
 			break;
 		retval = usb_parse_endpoint(ddev, cfgno, config, inum, asnum,
-				alt, num_ep, buffer, size);
+				alt, num_ep, buffer, size, &valid);
 		if (retval < 0)
 			return retval;
-		++n;
+		if (valid)
+			++n;
 
 		buffer += retval;
 		size -= retval;
-- 
2.44.0

Re: [RFC 5/6] usb: usb_parse_endpoint must not count duplicated endpoints

[Alan Stern]

On Thu, Apr 11, 2024 at 02:43:03PM +0200, Oliver Neukum wrote:
> When an interface is parsed the number of endpoints claimed to exist
> is compared to the number of endpoint descriptors actually found.
> Duplicated endpoints are not parsed in usb_parse_endpoint but
> usb_parse_interface counts them. That makes no sense.

It _does_ make sense.  If there are 4 endpoint descriptors in the buffer 
then "the number of endpoint descriptors actually found" is 4, even if 
some of them are duplicates.

> To correct this usb_parse_endpoint needs to return feedback
> about the validity of parsed endpoints.

If you make this change then the following error message:

		dev_notice(ddev, "config %d interface %d altsetting %d has %d "
		    "endpoint descriptor%s, different from the interface "
		    "descriptor's value: %d\n",
		    cfgno, inum, asnum, n, plural(n), num_ep_orig);

would be wrong, since n would not be the number of endpoint descriptors 
but rather the number of descriptors after duplicates were removed.

This does not need to be changed.

Alan Stern

> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/config.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 055910fc6b19..50acc9021247 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -254,7 +254,7 @@ static bool config_endpoint_is_duplicate(struct usb_host_config *config,
>  static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  		struct usb_host_config *config, int inum, int asnum,
>  		struct usb_host_interface *ifp, int num_ep,
> -		unsigned char *buffer, int size)
> +		unsigned char *buffer, int size, bool *valid)
>  {
>  	struct usb_device *udev = to_usb_device(ddev);
>  	unsigned char *buffer0 = buffer;
> @@ -270,6 +270,7 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  
>  	buffer += d->bLength;
>  	size -= d->bLength;
> +	*valid = false;
>  
>  	if (d->bDescriptorType != USB_DT_ENDPOINT)
>  		goto skip_to_next_endpoint_or_interface_descriptor;
> @@ -313,6 +314,7 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  		}
>  	}
>  
> +	*valid = true;
>  	endpoint = &ifp->endpoint[ifp->desc.bNumEndpoints];
>  	++ifp->desc.bNumEndpoints;
>  
> @@ -581,14 +583,17 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
>  	/* Parse all the endpoint descriptors */
>  	n = 0;
>  	while (size >= sizeof(struct usb_descriptor_header)) { /* minimum length to get bDescriptorType */
> +		bool valid;
> +
>  		if (((struct usb_descriptor_header *) buffer)->bDescriptorType
>  		     == USB_DT_INTERFACE)
>  			break;
>  		retval = usb_parse_endpoint(ddev, cfgno, config, inum, asnum,
> -				alt, num_ep, buffer, size);
> +				alt, num_ep, buffer, size, &valid);
>  		if (retval < 0)
>  			return retval;
> -		++n;
> +		if (valid)
> +			++n;
>  
>  		buffer += retval;
>  		size -= retval;
> -- 
> 2.44.0
> 
> 

[RFC 6/6] usb: config: find_next_descriptor can overflow buffer

[Oliver Neukum]

If you parse a data structure you cannot
just test whether the remainder of your buffer holds
data. It needs to hold a full data structure.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 50acc9021247..43c5ed256e6e 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -32,7 +32,7 @@ static int find_next_descriptor(unsigned char *buffer, int size,
 	unsigned char *buffer0 = buffer;
 
 	/* Find the next descriptor of type dt1 or dt2 */
-	while (size > 0) {
+	while (size >= sizeof(struct usb_descriptor_header)) {
 		h = (struct usb_descriptor_header *) buffer;
 		if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
 			break;
-- 
2.44.0

Re: [RFC 6/6] usb: config: find_next_descriptor can overflow buffer

[Alan Stern]

On Thu, Apr 11, 2024 at 02:43:04PM +0200, Oliver Neukum wrote:
> If you parse a data structure you cannot
> just test whether the remainder of your buffer holds
> data. It needs to hold a full data structure.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 50acc9021247..43c5ed256e6e 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -32,7 +32,7 @@ static int find_next_descriptor(unsigned char *buffer, int size,
>  	unsigned char *buffer0 = buffer;
>  
>  	/* Find the next descriptor of type dt1 or dt2 */
> -	while (size > 0) {
> +	while (size >= sizeof(struct usb_descriptor_header)) {
>  		h = (struct usb_descriptor_header *) buffer;
>  		if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
>  			break;

In fact, I don't think this is needed at all.  These checks are already 
present in usb_parse_configuration().

Alan Stern

Re: Hardening the parser during enumerations

[Greg KH]

On Thu, Apr 11, 2024 at 02:42:58PM +0200, Oliver Neukum wrote:
> The parser we use to enumerate a new device has no hardening
> against nonsensical descriptors at all. This is a bit optimistic
> 

When it was written, we trusted hardware, all we had to do was get it
working properly as all USB devices were supposed to have passed the
USB-IF's validation before it got to us.

Right now, we barely trust USB descriptors, if we wish to change this
threat-model, that's great, but I think a lot of work is still to be
done as you prove here.

thanks,

greg k-h

Re: Hardening the parser during enumerations

[Oliver Neukum]

On 11.04.24 16:09, Greg KH wrote:

> Right now, we barely trust USB descriptors, if we wish to change this
> threat-model, that's great, but I think a lot of work is still to be
> done as you prove here.

Indeed. As this is fiddly and holes are easy to overlook,
anything I've missed?

	Regards
		Oliver

Re: Hardening the parser during enumerations

[Greg KH]

On Thu, Apr 11, 2024 at 05:37:13PM +0200, Oliver Neukum wrote:
> On 11.04.24 16:09, Greg KH wrote:
> 
> > Right now, we barely trust USB descriptors, if we wish to change this
> > threat-model, that's great, but I think a lot of work is still to be
> > done as you prove here.
> 
> Indeed. As this is fiddly and holes are easy to overlook,
> anything I've missed?

We have had loads of fuzzing on the basic "parse the descriptors" logic,
so that's looking much better than before.  If you have actual
test-cases for the series you have here, that would help as well (we
need to integrate the syzbot usb descriptor fuzzing logic into kselftest
one of these days) so that both you can test if the changes are needed
(as Alan is pointing out they might not be), and that we can ensure that
future changes do not break anything.

But once a driver takes over for the device, all bets are off, we are
just now possibly hope that the endpoint assignment logic in drivers are
correct (so any help there is always appreciated), but after that, the
size of the endpoints and other basic protocol handling is fully
"trusted" and odds are no error checking is happening anywhere in almost
any driver.

So that means you still can not treat USB devices as "untrusted" sorry.
Just like any other hardware device in Linux.  So the threat-model is
the same, we have to trust the hardware.

thanks,

greg k-h