doubts concerning error handling in usb_parse_interface()

doubts concerning error handling in usb_parse_interface()

[Oliver Neukum]

Hi,

while looking at this strange CVE
https://github.com/wanrenmi/a-usb-kernel-bug?tab=readme-ov-file

I came to look at usb_parse_interface():

         /* Parse all the endpoint descriptors */
         n = 0;
         while (size > 0) {
                 if (((struct usb_descriptor_header *) buffer)->bDescriptorType
                      == USB_DT_INTERFACE)
                         break;
                 retval = usb_parse_endpoint(ddev, cfgno, config, inum, asnum,
                                 alt, num_ep, buffer, size);
                 if (retval < 0)
                         return retval;

If this fails catastrophically, we bail out

                 ++n;

If not, we count the endpoint as success

                 buffer += retval;
                 size -= retval;
         }

         if (n != num_ep_orig)
                 dev_notice(ddev, "config %d interface %d altsetting %d has %d "
                     "endpoint descriptor%s, different from the interface "
                     "descriptor's value: %d\n",
                     cfgno, inum, asnum, n, plural(n), num_ep_orig);
         return buffer - buffer0;

However, looking at usb_parse_endpoint() no errors are returned.
Should the check just go or have we dropped something important?
This code looks quite suspect, as we happily count duplicated endpoints
and endpoints with invalid addresses as successes.

	Regards
		Oliver

Re: doubts concerning error handling in usb_parse_interface()

[Alan Stern]

On Wed, Mar 27, 2024 at 03:16:24PM +0100, Oliver Neukum wrote:
> Hi,
> 
> while looking at this strange CVE
> https://github.com/wanrenmi/a-usb-kernel-bug?tab=readme-ov-file
> 
> I came to look at usb_parse_interface():
> 
>         /* Parse all the endpoint descriptors */
>         n = 0;
>         while (size > 0) {
>                 if (((struct usb_descriptor_header *) buffer)->bDescriptorType
>                      == USB_DT_INTERFACE)
>                         break;
>                 retval = usb_parse_endpoint(ddev, cfgno, config, inum, asnum,
>                                 alt, num_ep, buffer, size);
>                 if (retval < 0)
>                         return retval;
> 
> If this fails catastrophically, we bail out
> 
>                 ++n;
> 
> If not, we count the endpoint as success

Not as a success -- see below.

>                 buffer += retval;
>                 size -= retval;
>         }
> 
>         if (n != num_ep_orig)
>                 dev_notice(ddev, "config %d interface %d altsetting %d has %d "
>                     "endpoint descriptor%s, different from the interface "
>                     "descriptor's value: %d\n",
>                     cfgno, inum, asnum, n, plural(n), num_ep_orig);
>         return buffer - buffer0;
> 
> However, looking at usb_parse_endpoint() no errors are returned.
> Should the check just go or have we dropped something important?
> This code looks quite suspect, as we happily count duplicated endpoints
> and endpoints with invalid addresses as successes.

True, the return value isn't an error code.  The two lines that do the 
checking should be removed.

Each time usb_parse_endpoint() is called, it consumes one endpoint 
descriptor.  Maybe an invalid or duplicate descriptor, but still an 
endpoint descriptor.  So n (in the caller) is the total number of 
endpoint descriptors encountered.  The actual number of endpoints we 
use, generally a different number, is stored in alt->desc.bNumEndpoints.

Alan Stern