revisiting the issue of hardening the USB enumeration parser

[Oliver Neukum <oneukum@xxxxxxxx>]

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

From 7c18f5673ae416027b813a51ece4e689b4383a6c Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
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@xxxxxxxx>
---
 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 3362af165ef5..65d23fa19b3c 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -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