Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i787e41f1:Fastmail
Date:   Sat, 1 Apr 2023 16:53:17 +0200
From:   Greg KH <greg@kroah.com>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Alan Stern <stern@rowland.harvard.edu>,
        syzbot <syzbot+4b3f8190f6e13b3efd74@syzkaller.appspotmail.com>,
        syzbot <syzbot+1cb937c125adb93fad2d@syzkaller.appspotmail.com>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in
 shark_write_val/usb_submit_urb
Message-ID: <2023040148-aground-cornbread-84e2@gregkh>
References: <00000000000096e4f905f81b2702@google.com>
 <e382763c-cf33-4871-a761-1ac85ae36f27@rowland.harvard.edu>
 <8896f261-9602-4663-aa87-1feb9bf3ec0f@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8896f261-9602-4663-aa87-1feb9bf3ec0f@redhat.com>
Precedence: bulk

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@redhat.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.

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.

thanks,

greg k-h