| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2023040544-cuddly-glancing-f577@gregkh>
Date: Wed, 5 Apr 2023 16:44:37 +0200
From: Greg KH <greg@...ah.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Hans de Goede <hdegoede@...hat.com>,
syzbot <syzbot+4b3f8190f6e13b3efd74@...kaller.appspotmail.com>,
syzbot <syzbot+1cb937c125adb93fad2d@...kaller.appspotmail.com>,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in
shark_write_val/usb_submit_urb
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@...hat.com>
> > >
> > > 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
Powered by blists - more mailing lists