| Message ID | 20240411124722.17343-2-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/6] usb: usb_parse_endpoint ignore reserved bits | expand |
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
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
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
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
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
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; }
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(-)