From 7c18f5673ae416027b813a51ece4e689b4383a6c Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 16 May 2024 15:06:34 +0200
Subject: [PATCH] USB: find_next_descriptor: prevent eternal loop by misformed
descriptors
In find_next_descriptor() is called to operate on chains of descriptors.
The callers make sure that the chain as a whole is of sufficient length
and it is ensured that the buffer is not overflown.
However, the central does not guard against an inner link in the chain
claiming zero length. In that case the loop will make no progress and
go on forever. This case has to be tested for.
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/core/config.c | 2 ++
1 file changed, 2 insertions(+)
@@ -34,6 +34,8 @@ static int find_next_descriptor(unsigned char *buffer, int size,
/* Find the next descriptor of type dt1 or dt2 */
while (size > 0) {
h = (struct usb_descriptor_header *) buffer;
+ if (!h->bLength) /* we would loop forever */
+ return 0;
if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
break;
buffer += h->bLength;
--
2.45.0
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. So how about the attached patch? Regards Oliver