On Thu, May 16, 2024 at 03:48:41PM +0200, Oliver Neukum wrote:
> Hi,
>
> you convinced me that my last attempt to look at the parser
> was fundamentally flawed. Instead I went top down from parsing
> the configuration down to endpoints. I found one major issue.
>
> static int find_next_descriptor(unsigned char *buffer, int size,
> int dt1, int dt2, int *num_skipped)
> {
> struct usb_descriptor_header *h;
> int n = 0;
> unsigned char *buffer0 = buffer;
> /* Find the next descriptor of type dt1 or dt2 */
> while (size > 0) {
> h = (struct usb_descriptor_header *) buffer;
>
> if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
> break;
> buffer += h->bLength;
> size -= h->bLength;
> ++n;
> }
> /* Store the number of descriptors skipped and return the
> * number of bytes skipped */
> if (num_skipped)
> *num_skipped = n;
> return buffer - buffer0;
> }
>
> This is called from multiple sites on chains of descriptors.
> We do have a check for overflowing the buffer in the while statement.
> However, there is no guarantee for make progress. If at some point
> in the chain we arrive at a descriptor of neither type we are looking
> for and a bLength of 0, size will remain constant and the loop
> will go on forever.
>
> AFAICT this is guarded nowhere outside the function against.
You didn't notice this code in usb_parse_configuration() (starting
around line 659):
header = (struct usb_descriptor_header *) buffer2;
if ((header->bLength > size2) || (header->bLength < 2)) {
dev_notice(ddev, "config %d has an invalid descriptor "
"of length %d, skipping remainder of the config\n",
cfgno, header->bLength);
break;
}
The inner parentheses in the "if" condition aren't necessary, but the
second half of the condition protects against zero-length descriptors.
> So how about the attached patch?
It's not necessary.
Alan Stern
