| Message ID | 20240411124722.17343-3-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: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
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
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;
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(-)