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
