On Sat, Apr 01, 2023 at 02:38:39PM -0400, Alan Stern wrote: > On Sat, Apr 01, 2023 at 04:53:17PM +0200, Greg KH wrote: > > On Sat, Apr 01, 2023 at 12:48:07PM +0200, Hans de Goede wrote: > > > Hi Alan, > > > > > > On 3/30/23 22:10, Alan Stern wrote: > > > > Reference: https://syzkaller.appspot.com/bug?extid=4b3f8190f6e13b3efd74 > > > > Reference: https://syzkaller.appspot.com/bug?extid=1cb937c125adb93fad2d > > > > > > > > The radio-shark driver just assumes that the endpoints it uses will be > > > > present, without checking. This adds an appropriate check. > > > > > > > > Alan Stern > > > > > > > > #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2 > > > > > > Thank you for working on this! > > > > > > Both the core changes and the 2 radio-shark driver changes look good to me. > > > > > > Please add my: > > > > > > Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > > > > > When submitting these upstream. > > > > > > Regards, > > > > > > Hans > > > > > > > > > > > > > > > > > > > > > > > drivers/usb/core/usb.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/usb.h | 7 ++++ > > > > 2 files changed, 77 insertions(+) > > > > > > > > Index: usb-devel/drivers/usb/core/usb.c > > > > =================================================================== > > > > --- usb-devel.orig/drivers/usb/core/usb.c > > > > +++ usb-devel/drivers/usb/core/usb.c > > > > @@ -207,6 +207,76 @@ int usb_find_common_endpoints_reverse(st > > > > EXPORT_SYMBOL_GPL(usb_find_common_endpoints_reverse); > > > > > > > > /** > > > > + * usb_find_endpoint() - Given an endpoint address, search for the endpoint's > > > > + * usb_host_endpoint structure in an interface's current altsetting. > > > > + * @intf: the interface whose current altsetting should be searched > > > > + * @ep_addr: the endpoint address (number and direction) to find > > > > + * > > > > + * Search the altsetting's list of endpoints for one with the specified address. > > > > + * > > > > + * Return: Pointer to the usb_host_endpoint if found, %NULL otherwise. > > > > + */ > > > > +struct usb_host_endpoint __must_check *usb_find_endpoint( > > > > + const struct usb_interface *intf, unsigned int ep_addr) > > > > +{ > > > > + int n; > > > > + struct usb_host_endpoint *ep; > > > > + > > > > + n = intf->cur_altsetting->desc.bNumEndpoints; > > > > + ep = intf->cur_altsetting->endpoint; > > > > + for (; n > 0; (--n, ++ep)) { > > > > + if (ep->desc.bEndpointAddress == ep_addr) > > > > + return ep; > > > > + } > > > > + return NULL; > > > > +} > > > > +EXPORT_SYMBOL_GPL(usb_find_endpoint); > > > > + > > > > +/** > > > > + * usb_check_bulk_endpoint - Check whether an interface's current altsetting > > > > + * contains a bulk endpoint with the given address. > > > > + * @intf: the interface whose current altsetting should be searched > > > > + * @ep_addr: the endpoint address (number and direction) to look for > > > > + * > > > > + * Search for an endpoint with the specified address and check its type. > > > > + * > > > > + * Return: %true if the endpoint is found and is bulk, %false otherwise. > > > > + */ > > > > +bool usb_check_bulk_endpoint( > > > > + const struct usb_interface *intf, unsigned int ep_addr) > > > > +{ > > > > + const struct usb_host_endpoint *ep; > > > > + > > > > + ep = usb_find_endpoint(intf, ep_addr); > > > > + if (!ep) > > > > + return false; > > > > + return usb_endpoint_xfer_bulk(&ep->desc); > > > > +} > > > > +EXPORT_SYMBOL_GPL(usb_check_bulk_endpoint); > > > > + > > > > +/** > > > > + * usb_check_int_endpoint - Check whether an interface's current altsetting > > > > + * contains an interrupt endpoint with the given address. > > > > + * @intf: the interface whose current altsetting should be searched > > > > + * @ep_addr: the endpoint address (number and direction) to look for > > > > + * > > > > + * Search for an endpoint with the specified address and check its type. > > > > + * > > > > + * Return: %true if the endpoint is found and is interrupt, %false otherwise. > > > > + */ > > > > +bool usb_check_int_endpoint( > > > > + const struct usb_interface *intf, unsigned int ep_addr) > > > > +{ > > > > + const struct usb_host_endpoint *ep; > > > > + > > > > + ep = usb_find_endpoint(intf, ep_addr); > > > > + if (!ep) > > > > + return false; > > > > + return usb_endpoint_xfer_int(&ep->desc); > > > > +} > > > > +EXPORT_SYMBOL_GPL(usb_check_int_endpoint); > > > > Shouldn't you use the usb_find_bulk_in_endpoint() and friends functions > > instead of these? Many drivers hard-coded their "I know this endpoint > > is this type" which breaks in fuzzing as you know (and see here), which > > is why those functions were created to be used. > > It's true, we could use the existing functions in this case. I'm not sure > which approach would be better; it's probably mostly a matter of taste. > > Using the existing functions, the probe function would get additional code > something like this: > > struct usb_endpoint_descriptor *ep; > > if (!(usb_find_int_in_endpoint(intf->cur_altsetting, &ep) == 0 && > usb_endpoint_num(&ep->desc) == SHARK_IN_EP) || > !(usb_find_int_out_endpoint(intf->cur_altsetting, &ep) == 0 && > usb_endpoint_num(&ep->desc) == SHARK_OUT_EP)) { > dev_err(... > > Using the new functions (with a revised API, see the patch below) we would > instead add this: > > static const u8 ep_addresses[] = { > SHARK_IN_EP | USB_DIR_IN, > SHARK_OUT_EP | USB_DIR_OUT, > 0}; > > if (!usb_check_int_endpoints(intf, ep_addresses)) { > dev_err(... > > In this case the difference is not particularly meaningful. With the new > functions, the amount of code added to the driver is smaller, but of > course that's offset by adding the new functions themselves to the core. > (On the other hand there's probably a bunch of drivers that could stand > to be fixed up this way, which would amortize the cost to the core.) > > The difference becomes a lot more noticeable with the sisusbvga driver > [1]. It has six endpoints that need to be checked: four bulk-OUT and two > bulk-IN. While usb_find_common_endpoints() would be able to find them, > it's not well suited for checking that they have the expected addresses. > With the new functions, all the work can be done with a single call. > > > I think just using them in the probe function would fix this issue > > instead of these functions which would only be used by that one driver. > > It wouldn't be used just by these two drivers. The new routines are > ideally suited for doing the checking in old drivers that have their > endpoint numbers and directions built-in, like these do. While I haven't > looked to see, there must be quite a few of them. Ok, fair enough, let's take this and see what other drivers can be fixed up this way. thanks, greg k-h
