revisiting the issue of hardening the USB enumeration parser

Commit Message

Oliver Neukum May 16, 2024, 1:48 p.m. UTC

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

Comments

Alan Stern May 16, 2024, 4:58 p.m. UTC | #1

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

Patch

From 7c18f5673ae416027b813a51ece4e689b4383a6c Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[e-mail]>
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 <[e-mail]>
---
 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