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;
Message-ID: <8896f261-9602-4663-aa87-1feb9bf3ec0f@redhat.com>
Date:   Sat, 1 Apr 2023 12:48:07 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in
 shark_write_val/usb_submit_urb
To:     Alan Stern <stern@rowland.harvard.edu>,
        syzbot <syzbot+4b3f8190f6e13b3efd74@syzkaller.appspotmail.com>,
        syzbot <syzbot+1cb937c125adb93fad2d@syzkaller.appspotmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
References: <00000000000096e4f905f81b2702@google.com>
 <e382763c-cf33-4871-a761-1ac85ae36f27@rowland.harvard.edu>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <e382763c-cf33-4871-a761-1ac85ae36f27@rowland.harvard.edu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);
> +
> +/**
>   * usb_find_alt_setting() - Given a configuration, find the alternate setting
>   * for the given interface.
>   * @config: the configuration to search (not necessarily the current config).
> Index: usb-devel/include/linux/usb.h
> ===================================================================
> --- usb-devel.orig/include/linux/usb.h
> +++ usb-devel/include/linux/usb.h
> @@ -292,6 +292,13 @@ void usb_put_intf(struct usb_interface *
>  #define USB_MAXINTERFACES	32
>  #define USB_MAXIADS		(USB_MAXINTERFACES/2)
>  
> +struct usb_host_endpoint __must_check *usb_find_endpoint(
> +		const struct usb_interface *intf, unsigned int ep_addr);
> +bool usb_check_bulk_endpoint(
> +		const struct usb_interface *intf, unsigned int ep_addr);
> +bool usb_check_int_endpoint(
> +		const struct usb_interface *intf, unsigned int ep_addr);
> +
>  /*
>   * USB Resume Timer: Every Host controller driver should drive the resume
>   * signalling on the bus for the amount of time defined by this macro.
> 
>  drivers/media/radio/radio-shark.c  |    7 +++++++
>  drivers/media/radio/radio-shark2.c |    7 +++++++
>  2 files changed, 14 insertions(+)
> 
> Index: usb-devel/drivers/media/radio/radio-shark.c
> ===================================================================
> --- usb-devel.orig/drivers/media/radio/radio-shark.c
> +++ usb-devel/drivers/media/radio/radio-shark.c
> @@ -317,6 +317,13 @@ static int usb_shark_probe(struct usb_in
>  	struct shark_device *shark;
>  	int retval = -ENOMEM;
>  
> +	/* Are the expected endpoints present? */
> +	if (!usb_check_int_endpoint(intf, SHARK_IN_EP | USB_DIR_IN) ||
> +	    !usb_check_int_endpoint(intf, SHARK_OUT_EP | USB_DIR_OUT)) {
> +		dev_err(&intf->dev, "Invalid radioSHARK device\n");
> +		return -EINVAL;
> +	}
> +
>  	shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
>  	if (!shark)
>  		return retval;
> Index: usb-devel/drivers/media/radio/radio-shark2.c
> ===================================================================
> --- usb-devel.orig/drivers/media/radio/radio-shark2.c
> +++ usb-devel/drivers/media/radio/radio-shark2.c
> @@ -283,6 +283,13 @@ static int usb_shark_probe(struct usb_in
>  	struct shark_device *shark;
>  	int retval = -ENOMEM;
>  
> +	/* Are the expected endpoints present? */
> +	if (!usb_check_int_endpoint(intf, SHARK_IN_EP | USB_DIR_IN) ||
> +	    !usb_check_int_endpoint(intf, SHARK_OUT_EP | USB_DIR_OUT)) {
> +		dev_err(&intf->dev, "Invalid radioSHARK2 device\n");
> +		return -EINVAL;
> +	}
> +
>  	shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
>  	if (!shark)
>  		return retval;
>