[syzbot] Monthly usb report

[syzbot] Monthly usb report

[syzbot]

Hello usb maintainers/developers,

This is a 30-day syzbot report for the usb subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/usb

During the period, 1 new issues were detected and 0 were fixed.
In total, 32 issues are still open and 136 have been fixed so far.

Some of the still happening issues:

Crashes Repro Title
1238    Yes   KMSAN: uninit-value in mii_nway_restart
              https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955
413     Yes   WARNING in sisusb_send_bulk_msg/usb_submit_urb
              https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79
344     No    INFO: task hung in usb_get_descriptor (2)
              https://syzkaller.appspot.com/bug?extid=e8db9d9e65feff8fa471
86      No    INFO: task hung in hub_event (3)
              https://syzkaller.appspot.com/bug?extid=a7edecbf389d11a369d4
60      Yes   WARNING in shark_write_reg/usb_submit_urb
              https://syzkaller.appspot.com/bug?extid=4b3f8190f6e13b3efd74
50      Yes   WARNING in shark_write_val/usb_submit_urb
              https://syzkaller.appspot.com/bug?extid=1cb937c125adb93fad2d
31      No    INFO: task hung in hub_port_init (3)
              https://syzkaller.appspot.com/bug?extid=b6f11035e572f08bc20f

---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Alan Stern]

Reference: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79

The sisusbvga driver just assumes that the endpoints it uses will be 
present, without checking.  I don't know anything about this driver, so 
the fix below may not be entirely correct.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2

--- usb-devel.orig/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ usb-devel/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -2772,6 +2772,24 @@ static struct usb_class_driver usb_sisus
 	.minor_base =	SISUSB_MINOR
 };
 
+/*
+ * Check whether the current altsetting for intf contains a bulk endpoint
+ * with the specified address (number and direction).
+ */
+static int check_bulk_ep(struct usb_interface *intf, unsigned int ep_addr)
+{
+	int n, i;
+	const struct usb_endpoint_descriptor *epd;
+
+	n = intf->cur_altsetting->desc.bNumEndpoints;
+	for (i = 0; i < n; ++i) {
+		epd = &intf->cur_altsetting->endpoint[i].desc;
+		if (epd->bEndpointAddress == ep_addr)
+			return usb_endpoint_xfer_bulk(epd);
+	}
+	return 0;
+}
+
 static int sisusb_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
@@ -2779,6 +2797,17 @@ static int sisusb_probe(struct usb_inter
 	struct sisusb_usb_data *sisusb;
 	int retval = 0, i;
 
+	/* Are the expected endpoints present? */
+	if (!check_bulk_ep(intf, SISUSB_EP_GFX_IN | USB_DIR_IN) ||
+	    !check_bulk_ep(intf, SISUSB_EP_GFX_OUT | USB_DIR_OUT) ||
+	    !check_bulk_ep(intf, SISUSB_EP_GFX_BULK_OUT | USB_DIR_OUT) ||
+	    !check_bulk_ep(intf, SISUSB_EP_GFX_LBULK_OUT | USB_DIR_OUT) ||
+	    !check_bulk_ep(intf, SISUSB_EP_BRIDGE_IN | USB_DIR_IN) ||
+	    !check_bulk_ep(intf, SISUSB_EP_BRIDGE_OUT | USB_DIR_OUT)) {
+		dev_err(&dev->dev, "Invalid USB2VGA device\n");
+		return -EINVAL;
+	}
+
 	dev_info(&dev->dev, "USB2VGA dongle found at address %d\n",
 			dev->devnum);
 

Re: [syzbot] [usb?] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+23be03b56c5259385d79@syzkaller.appspotmail.com

Tested on:

commit:         c9c3395d Linux 6.2
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2
console output: https://syzkaller.appspot.com/x/log.txt?x=16fe503ec80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=8b64e70ff2a55d53
dashboard link: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=10b3a60dc80000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Oliver Neukum]

On 30.03.23 17:34, Alan Stern wrote:
> Reference: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79
> 
> The sisusbvga driver just assumes that the endpoints it uses will be
> present, without checking.  I don't know anything about this driver, so
> the fix below may not be entirely correct.

Hi,

this patch by itself looks good to me.

But the need for it is problematic. Do we have any vendor specific driver
that could get away without an equivalent to this patch without showing
an equivalent bug? If so, why do we have a generic matching code, although
it is always insufficient?

What is the purpose of a generic binding interface in sysfs if every probe()
method blocks it? Allowing a generic probe looks like a misdesign under these
circumstances. You'd really want to add IDs to drivers.

	Regards
		Oliver

Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Alan Stern]

On Mon, Apr 03, 2023 at 10:54:05AM +0200, Oliver Neukum wrote:
> 
> 
> On 30.03.23 17:34, Alan Stern wrote:
> > Reference: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79
> > 
> > The sisusbvga driver just assumes that the endpoints it uses will be
> > present, without checking.  I don't know anything about this driver, so
> > the fix below may not be entirely correct.
> 
> Hi,
> 
> this patch by itself looks good to me.
> 
> But the need for it is problematic. Do we have any vendor specific driver
> that could get away without an equivalent to this patch without showing
> an equivalent bug?

Probably not.  Which is why adding this checking infrastructure to the 
USB core seems like a good idea, even though implementing it in each of 
the vendor-specific drivers may take quite a while.

>  If so, why do we have a generic matching code, although
> it is always insufficient?

(I assume you're referring to usb_match_device() and related routines.)

Not sufficient, perhaps, but necessary.  That is, in addition to 
checking the available endpoints, we also have to make sure the device 
has the right vendor ID, product ID, and so on to match the driver.

> What is the purpose of a generic binding interface in sysfs if every probe()
> method blocks it? Allowing a generic probe looks like a misdesign under these
> circumstances. You'd really want to add IDs to drivers.

I really don't understand what you're asking.  If you're talking about 
the "bind" and "unbind" files in /sys/bus/*/drivers/*/, they are there 
to allow manual binding and unbinding of devices.  Even though only one 
driver is likely to bind to any particular device.  (Furthermore, all 
this was true even before we started being careful about checking 
endpoints numbers and types.)

And we _do_ have IDs in drivers; that's what the .id_table member of 
struct usb_driver is for.

Alan Stern

Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Oliver Neukum]

On 03.04.23 16:33, Alan Stern wrote:
> On Mon, Apr 03, 2023 at 10:54:05AM +0200, Oliver Neukum wrote:
>>
>>
>> On 30.03.23 17:34, Alan Stern wrote:

>>   If so, why do we have a generic matching code, although
>> it is always insufficient?
> 
> (I assume you're referring to usb_match_device() and related routines.)
> 
> Not sufficient, perhaps, but necessary.  That is, in addition to
> checking the available endpoints, we also have to make sure the device
> has the right vendor ID, product ID, and so on to match the driver.

The thing is that if we also need to check in each driver, the criteria
for matching devices are not sophisticated and strict enough
> 
>> What is the purpose of a generic binding interface in sysfs if every probe()
>> method blocks it? Allowing a generic probe looks like a misdesign under these
>> circumstances. You'd really want to add IDs to drivers.
> 
> I really don't understand what you're asking.  If you're talking about
> the "bind" and "unbind" files in /sys/bus/*/drivers/*/, they are there

Yes

> to allow manual binding and unbinding of devices.  Even though only one
> driver is likely to bind to any particular device.  (Furthermore, all

They, however, allow binding drivers to arbitrary devices.
Now, you can argue that this will not work. Then I'd say that the correct interface
would be per device, not per driver as it is now and would retrigger
a binding, as if the device were new.
Or you say that if the administrator wants that, well, shoot
yourself into the foot, the driver shall not check.

> this was true even before we started being careful about checking
> endpoints numbers and types.)
> 
> And we _do_ have IDs in drivers; that's what the .id_table member of
> struct usb_driver is for.

Which is not exported through sysfs.
So we export an interface that is not fully usable, not the one people
really want. You almost never want to say that a device at a specific
port is to be bound to a driver at one specific time.
You want to either assign all devices with a new ID to a driver
or unbind and reprobe a device.

	Regards
		Oliver

Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Alan Stern]

On Mon, Apr 03, 2023 at 04:51:26PM +0200, Oliver Neukum wrote:
> 
> 
> On 03.04.23 16:33, Alan Stern wrote:
> > On Mon, Apr 03, 2023 at 10:54:05AM +0200, Oliver Neukum wrote:
> > > 
> > > 
> > > On 30.03.23 17:34, Alan Stern wrote:
> 
> > >   If so, why do we have a generic matching code, although
> > > it is always insufficient?
> > 
> > (I assume you're referring to usb_match_device() and related routines.)
> > 
> > Not sufficient, perhaps, but necessary.  That is, in addition to
> > checking the available endpoints, we also have to make sure the device
> > has the right vendor ID, product ID, and so on to match the driver.
> 
> The thing is that if we also need to check in each driver, the criteria
> for matching devices are not sophisticated and strict enough

Why not?  Part of the check takes place in the core and part in the 
driver.  The fact that the checking is done in two parts doesn't mean it 
is unsophisticated or not strict enough.

In addition, one can argue that only the checking done in the core 
should be called "matching".  If the match succeeds then it is 
appropriate to try binding the device to the matching driver.  If the 
driver then refuses to accept the binding because (for example) the 
endpoints are wrong, it doesn't mean the match should have failed.  It 
means that the device is broken in some way and is therefore unusable.

> > > What is the purpose of a generic binding interface in sysfs if every probe()
> > > method blocks it? Allowing a generic probe looks like a misdesign under these
> > > circumstances. You'd really want to add IDs to drivers.
> > 
> > I really don't understand what you're asking.  If you're talking about
> > the "bind" and "unbind" files in /sys/bus/*/drivers/*/, they are there
> 
> Yes
> 
> > to allow manual binding and unbinding of devices.  Even though only one
> > driver is likely to bind to any particular device.  (Furthermore, all
> 
> They, however, allow binding drivers to arbitrary devices.

No.  The binding uses the normal matching and probing mechanism.  Here's 
the comment at the start of bind_store() in drivers/base/bus.c:

/*
 * Manually attach a device to a driver.
 * Note: the driver must want to bind to the device,
 * it is not possible to override the driver's id table.
 */

> Now, you can argue that this will not work. Then I'd say that the correct interface
> would be per device, not per driver as it is now and would retrigger
> a binding, as if the device were new.
> Or you say that if the administrator wants that, well, shoot
> yourself into the foot, the driver shall not check.

In view of the misunderstanding above, these comments are moot.

> > this was true even before we started being careful about checking
> > endpoints numbers and types.)
> > 
> > And we _do_ have IDs in drivers; that's what the .id_table member of
> > struct usb_driver is for.
> 
> Which is not exported through sysfs.

True, the built-in table is not exported (I guess we could add some sort 
of read-only view of it).  There is, however an additional dynamic ID 
table which _is_ managed through sysfs.

> So we export an interface that is not fully usable, not the one people
> really want. You almost never want to say that a device at a specific
> port is to be bound to a driver at one specific time.
> You want to either assign all devices with a new ID to a driver
> or unbind and reprobe a device.

The first can be done by adding a dynamic ID entry (or on a permanent 
basis by updating the driver's built-in table and rebuilding the 
driver's module), and the second can be done using the "unbind" and 
"bind" files.

Alan Stern

Re: [syzbot] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[Alan Stern]

The patch has been revised.  Make sure it still works right.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2

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,82 @@ 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.
+ */
+static const struct usb_host_endpoint *usb_find_endpoint(
+		const struct usb_interface *intf, unsigned int ep_addr)
+{
+	int n;
+	const 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;
+}
+
+/**
+ * usb_check_bulk_endpoints - Check whether an interface's current altsetting
+ * contains a set of bulk endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are bulk, %false otherwise.
+ */
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_bulk(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_bulk_endpoints);
+
+/**
+ * usb_check_int_endpoints - Check whether an interface's current altsetting
+ * contains a set of interrupt endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are interrupt,
+ * %false otherwise.
+ */
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_int(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_int_endpoints);
+
+/**
  * 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,11 @@ void usb_put_intf(struct usb_interface *
 #define USB_MAXINTERFACES	32
 #define USB_MAXIADS		(USB_MAXINTERFACES/2)
 
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+
 /*
  * USB Resume Timer: Every Host controller driver should drive the resume
  * signalling on the bus for the amount of time defined by this macro.

Index: usb-devel/drivers/usb/misc/sisusbvga/sisusbvga.c
===================================================================
--- usb-devel.orig/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ usb-devel/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -2778,6 +2778,20 @@ static int sisusb_probe(struct usb_inter
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct sisusb_usb_data *sisusb;
 	int retval = 0, i;
+	static const u8 ep_addresses[] = {
+		SISUSB_EP_GFX_IN | USB_DIR_IN,
+		SISUSB_EP_GFX_OUT | USB_DIR_OUT,
+		SISUSB_EP_GFX_BULK_OUT | USB_DIR_OUT,
+		SISUSB_EP_GFX_LBULK_OUT | USB_DIR_OUT,
+		SISUSB_EP_BRIDGE_IN | USB_DIR_IN,
+		SISUSB_EP_BRIDGE_OUT | USB_DIR_OUT,
+		0};
+
+	/* Are the expected endpoints present? */
+	if (!usb_check_bulk_endpoints(intf, ep_addresses)) {
+		dev_err(&intf->dev, "Invalid USB2VGA device\n");
+		return -EINVAL;
+	}
 
 	dev_info(&dev->dev, "USB2VGA dongle found at address %d\n",
 			dev->devnum);

Re: [syzbot] [usb?] WARNING in sisusb_send_bulk_msg/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+23be03b56c5259385d79@syzkaller.appspotmail.com

Tested on:

commit:         c9c3395d Linux 6.2
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2
console output: https://syzkaller.appspot.com/x/log.txt?x=13c2c303c80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=8b64e70ff2a55d53
dashboard link: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14649cc3c80000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in shark_write_val/usb_submit_urb

[Alan Stern]

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

 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;

Re: [syzbot] [usb?] WARNING in shark_write_reg/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+4b3f8190f6e13b3efd74@syzkaller.appspotmail.com

Tested on:

commit:         c9c3395d Linux 6.2
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2
console output: https://syzkaller.appspot.com/x/log.txt?x=16d9f695c80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=fea01b13d861cd1e
dashboard link: https://syzkaller.appspot.com/bug?extid=4b3f8190f6e13b3efd74
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1134ea95c80000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in shark_write_val/usb_submit_urb

[Hans de Goede]

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;
> 

Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in shark_write_val/usb_submit_urb

[Greg KH]

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

Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in shark_write_val/usb_submit_urb

[Alan Stern]

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

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.

Alan Stern

[1] https://lore.kernel.org/linux-usb/b799fc68-8840-43e7-85f5-27e1e6457a44@rowland.harvard.edu


 drivers/usb/core/usb.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h    |    5 +++
 2 files changed, 81 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,82 @@ 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;
+}
+
+/**
+ * usb_check_bulk_endpoints - Check whether an interface's current altsetting
+ * contains a set of bulk endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are bulk, %false otherwise.
+ */
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_bulk(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_bulk_endpoints);
+
+/**
+ * usb_check_int_endpoints - Check whether an interface's current altsetting
+ * contains a set of interrupt endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are interrupt,
+ * %false otherwise.
+ */
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_int(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_int_endpoints);
+
+/**
  * 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,11 @@ void usb_put_intf(struct usb_interface *
 #define USB_MAXINTERFACES	32
 #define USB_MAXIADS		(USB_MAXINTERFACES/2)
 
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+
 /*
  * USB Resume Timer: Every Host controller driver should drive the resume
  * signalling on the bus for the amount of time defined by this macro.

Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb, WARNING in shark_write_val/usb_submit_urb

[Greg KH]

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

[PATCH 1/3] USB: core: Add routines for endpoint checks in old drivers

[Alan Stern]

Many of the older USB drivers in the Linux USB stack were written
based simply on a vendor's device specification.  They use the
endpoint information in the spec and assume these endpoints will
always be present, with the properties listed, in any device matching
the given vendor and product IDs.

While that may have been true back then, with spoofing and fuzzing it
is not true any more.  More and more we are finding that those old
drivers need to perform at least a minimum of checking before they try
to use any endpoint other than ep0.

To make this checking as simple as possible, we now add a couple of
utility routines to the USB core.  usb_check_bulk_endpoints() and
usb_check_int_endpoints() take an interface pointer together with a
list of endpoint addresses (numbers and directions).  They check that
the interface's current alternate setting includes endpoints with
those addresses and that each of these endpoints has the right type:
bulk or interrupt, respectively.

Although we already have usb_find_common_endpoints() and related
routines meant for a similar purpose, they are not well suited for
this kind of checking.  Those routines find endpoints of various
kinds, but only one (either the first or the last) of each kind, and
they don't verify that the endpoints' addresses agree with what the
caller expects.

In theory the new routines could be more general: They could take a
particular altsetting as their argument instead of always using the
interface's current altsetting.  In practice I think this won't matter
too much; multiple altsettings tend to be used for transferring media
(audio or visual) over isochronous endpoints, not bulk or interrupt.
Drivers for such devices will generally require more sophisticated
checking than these simplistic routines provide.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

This patch and the one for the radio-shark drivers have changed since
Hans reviewed them, so I'm not including his Reviewed-by: tag.


[as1992]


 drivers/usb/core/usb.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h    |    5 +++
 2 files changed, 81 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,82 @@ 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.
+ */
+static const struct usb_host_endpoint *usb_find_endpoint(
+		const struct usb_interface *intf, unsigned int ep_addr)
+{
+	int n;
+	const 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;
+}
+
+/**
+ * usb_check_bulk_endpoints - Check whether an interface's current altsetting
+ * contains a set of bulk endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are bulk, %false otherwise.
+ */
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_bulk(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_bulk_endpoints);
+
+/**
+ * usb_check_int_endpoints - Check whether an interface's current altsetting
+ * contains a set of interrupt endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are interrupt,
+ * %false otherwise.
+ */
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_int(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_int_endpoints);
+
+/**
  * 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,11 @@ void usb_put_intf(struct usb_interface *
 #define USB_MAXINTERFACES	32
 #define USB_MAXIADS		(USB_MAXINTERFACES/2)
 
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+
 /*
  * USB Resume Timer: Every Host controller driver should drive the resume
  * signalling on the bus for the amount of time defined by this macro.

[PATCH 2/3] USB: sisusbvga: Add endpoint checks

[Alan Stern]

The syzbot fuzzer was able to provoke a WARNING from the sisusbvga driver:

------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 1 PID: 26 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
Modules linked in:
CPU: 1 PID: 26 Comm: kworker/1:1 Not tainted 6.2.0-rc5-syzkaller-00199-g5af6ce704936 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
Code: 7c 24 18 e8 6c 50 80 fb 48 8b 7c 24 18 e8 62 1a 01 ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 60 b1 fa 8a e8 84 b0 be 03 <0f> 0b e9 58 f8 ff ff e8 3e 50 80 fb 48 81 c5 c0 05 00 00 e9 84 f7
RSP: 0018:ffffc90000a1ed18 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
RDX: ffff888012783a80 RSI: ffffffff816680ec RDI: fffff52000143d95
RBP: ffff888079020000 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: 0000000000000003
R13: ffff888017d33370 R14: 0000000000000003 R15: ffff888021213600
FS:  0000000000000000(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005592753a60b0 CR3: 0000000022899000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 sisusb_bulkout_msg drivers/usb/misc/sisusbvga/sisusbvga.c:224 [inline]
 sisusb_send_bulk_msg.constprop.0+0x904/0x1230 drivers/usb/misc/sisusbvga/sisusbvga.c:379
 sisusb_send_bridge_packet drivers/usb/misc/sisusbvga/sisusbvga.c:567 [inline]
 sisusb_do_init_gfxdevice drivers/usb/misc/sisusbvga/sisusbvga.c:2077 [inline]
 sisusb_init_gfxdevice+0x87b/0x4000 drivers/usb/misc/sisusbvga/sisusbvga.c:2177
 sisusb_probe+0x9cd/0xbe2 drivers/usb/misc/sisusbvga/sisusbvga.c:2869
...

The problem was caused by the fact that the driver does not check
whether the endpoints it uses are actually present and have the
appropriate types.  This can be fixed by adding a simple check of
the endpoints.

Reported-and-tested-by: syzbot+23be03b56c5259385d79@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Link: https://syzkaller.appspot.com/bug?extid=23be03b56c5259385d79

---


[as1993]


 drivers/usb/misc/sisusbvga/sisusbvga.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: usb-devel/drivers/usb/misc/sisusbvga/sisusbvga.c
===================================================================
--- usb-devel.orig/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ usb-devel/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -2778,6 +2778,20 @@ static int sisusb_probe(struct usb_inter
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct sisusb_usb_data *sisusb;
 	int retval = 0, i;
+	static const u8 ep_addresses[] = {
+		SISUSB_EP_GFX_IN | USB_DIR_IN,
+		SISUSB_EP_GFX_OUT | USB_DIR_OUT,
+		SISUSB_EP_GFX_BULK_OUT | USB_DIR_OUT,
+		SISUSB_EP_GFX_LBULK_OUT | USB_DIR_OUT,
+		SISUSB_EP_BRIDGE_IN | USB_DIR_IN,
+		SISUSB_EP_BRIDGE_OUT | USB_DIR_OUT,
+		0};
+
+	/* Are the expected endpoints present? */
+	if (!usb_check_bulk_endpoints(intf, ep_addresses)) {
+		dev_err(&intf->dev, "Invalid USB2VGA device\n");
+		return -EINVAL;
+	}
 
 	dev_info(&dev->dev, "USB2VGA dongle found at address %d\n",
 			dev->devnum);

[PATCH 3/3] media: radio-shark: Add endpoint checks

[Alan Stern]

The syzbot fuzzer was able to provoke a WARNING from the radio-shark2
driver:

------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 1 != type 3
WARNING: CPU: 0 PID: 3271 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed2/0x1880 drivers/usb/core/urb.c:504
Modules linked in:
CPU: 0 PID: 3271 Comm: kworker/0:3 Not tainted 6.1.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0xed2/0x1880 drivers/usb/core/urb.c:504
Code: 7c 24 18 e8 00 36 ea fb 48 8b 7c 24 18 e8 36 1c 02 ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 a0 b6 90 8a e8 9a 29 b8 03 <0f> 0b e9 58 f8 ff ff e8 d2 35 ea fb 48 81 c5 c0 05 00 00 e9 84 f7
RSP: 0018:ffffc90003876dd0 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: ffff8880750b0040 RSI: ffffffff816152b8 RDI: fffff5200070edac
RBP: ffff8880172d81e0 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: 0000000000000001
R13: ffff8880285c5040 R14: 0000000000000002 R15: ffff888017158200
FS:  0000000000000000(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffe03235b90 CR3: 000000000bc8e000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 usb_start_wait_urb+0x101/0x4b0 drivers/usb/core/message.c:58
 usb_bulk_msg+0x226/0x550 drivers/usb/core/message.c:387
 shark_write_reg+0x1ff/0x2e0 drivers/media/radio/radio-shark2.c:88
...

The problem was caused by the fact that the driver does not check
whether the endpoints it uses are actually present and have the
appropriate types.  This can be fixed by adding a simple check of
these endpoints (and similarly for the radio-shark driver).

Reported-and-tested-by: syzbot+4b3f8190f6e13b3efd74@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Link: https://syzkaller.appspot.com/bug?extid=4b3f8190f6e13b3efd74

---


[as1994]


 drivers/media/radio/radio-shark.c  |   10 ++++++++++
 drivers/media/radio/radio-shark2.c |   10 ++++++++++
 2 files changed, 20 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
@@ -316,6 +316,16 @@ static int usb_shark_probe(struct usb_in
 {
 	struct shark_device *shark;
 	int retval = -ENOMEM;
+	static const u8 ep_addresses[] = {
+		SHARK_IN_EP | USB_DIR_IN,
+		SHARK_OUT_EP | USB_DIR_OUT,
+		0};
+
+	/* Are the expected endpoints present? */
+	if (!usb_check_int_endpoints(intf, ep_addresses)) {
+		dev_err(&intf->dev, "Invalid radioSHARK device\n");
+		return -EINVAL;
+	}
 
 	shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
 	if (!shark)
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
@@ -282,6 +282,16 @@ static int usb_shark_probe(struct usb_in
 {
 	struct shark_device *shark;
 	int retval = -ENOMEM;
+	static const u8 ep_addresses[] = {
+		SHARK_IN_EP | USB_DIR_IN,
+		SHARK_OUT_EP | USB_DIR_OUT,
+		0};
+
+	/* Are the expected endpoints present? */
+	if (!usb_check_int_endpoints(intf, ep_addresses)) {
+		dev_err(&intf->dev, "Invalid radioSHARK2 device\n");
+		return -EINVAL;
+	}
 
 	shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
 	if (!shark)

Re: [PATCH 1/3] USB: core: Add routines for endpoint checks in old drivers

[Oliver Neukum]

On 10.04.23 21:37, Alan Stern wrote:

Hi,
  
> To make this checking as simple as possible, we now add a couple of
> utility routines to the USB core.  usb_check_bulk_endpoints() and
> usb_check_int_endpoints() take an interface pointer together with a
> list of endpoint addresses (numbers and directions).  They check that
> the interface's current alternate setting includes endpoints with
> those addresses and that each of these endpoints has the right type:
> bulk or interrupt, respectively.
> 
> Although we already have usb_find_common_endpoints() and related
> routines meant for a similar purpose, they are not well suited for
> this kind of checking.  Those routines find endpoints of various
> kinds, but only one (either the first or the last) of each kind, and
> they don't verify that the endpoints' addresses agree with what the
> caller expects.

these will do the job. Yet this strikes me as unelegant. That is
if you define a data structure to match against, why not
add a pointer to it to struct usb_device_id and use that?

Basically the table of endpoints you are creating is a description of
a device. Why add code for checking it to each probe() method
that needs it?

	Regards
		Oliver

Re: [PATCH 1/3] USB: core: Add routines for endpoint checks in old drivers

[Alan Stern]

On Wed, Apr 12, 2023 at 01:54:12PM +0200, Oliver Neukum wrote:
> On 10.04.23 21:37, Alan Stern wrote:
> 
> Hi,
> > To make this checking as simple as possible, we now add a couple of
> > utility routines to the USB core.  usb_check_bulk_endpoints() and
> > usb_check_int_endpoints() take an interface pointer together with a
> > list of endpoint addresses (numbers and directions).  They check that
> > the interface's current alternate setting includes endpoints with
> > those addresses and that each of these endpoints has the right type:
> > bulk or interrupt, respectively.
> > 
> > Although we already have usb_find_common_endpoints() and related
> > routines meant for a similar purpose, they are not well suited for
> > this kind of checking.  Those routines find endpoints of various
> > kinds, but only one (either the first or the last) of each kind, and
> > they don't verify that the endpoints' addresses agree with what the
> > caller expects.
> 
> these will do the job. Yet this strikes me as unelegant. That is
> if you define a data structure to match against, why not
> add a pointer to it to struct usb_device_id and use that?

Struct usb_device_id doesn't seem like the right place.  Struct 
usb_driver would be more appropriate.  The drivers that need this have 
only one entry in their match table, which means that drivers with large 
match tables (which would require a lot of extra space for the new 
pointers) don't need it.

> Basically the table of endpoints you are creating is a description of
> a device. Why add code for checking it to each probe() method
> that needs it?

True, the checks could be centralized in usb_probe_interface().  What do 
you think about doing it that way?

Alan Stern

Re: [PATCH 1/3] USB: core: Add routines for endpoint checks in old drivers

[Oliver Neukum]

On 12.04.23 17:08, Alan Stern wrote:
> On Wed, Apr 12, 2023 at 01:54:12PM +0200, Oliver Neukum wrote:

>> these will do the job. Yet this strikes me as unelegant. That is
>> if you define a data structure to match against, why not
>> add a pointer to it to struct usb_device_id and use that?
> 
> Struct usb_device_id doesn't seem like the right place.  Struct

Conceptually it belongs there. It describes a device, not a driver.
I would say that the name is not well chosen, but it is the right place.

> usb_driver would be more appropriate.  The drivers that need this have
> only one entry in their match table, which means that drivers with large

Why would that be the case? As far as I can see everything that is not
a class driver will need this or an equivalent and many of them
support multiple types of devices.

> match tables (which would require a lot of extra space for the new
> pointers) don't need it.

A few dozen bytes.

> True, the checks could be centralized in usb_probe_interface().  What do
> you think about doing it that way?

That really is the best place to put the code for checking.
And you might put into a comment that the way USB works the endpoint
number includes the direction. It is not obvious to a casual reader.

	Regards
		Oliver

Re: [PATCH 1/3] USB: core: Add routines for endpoint checks in old drivers

[Alan Stern]

On Wed, Apr 12, 2023 at 08:52:45PM +0200, Oliver Neukum wrote:
> 
> 
> On 12.04.23 17:08, Alan Stern wrote:
> > On Wed, Apr 12, 2023 at 01:54:12PM +0200, Oliver Neukum wrote:
> 
> > > these will do the job. Yet this strikes me as unelegant. That is
> > > if you define a data structure to match against, why not
> > > add a pointer to it to struct usb_device_id and use that?
> > 
> > Struct usb_device_id doesn't seem like the right place.  Struct
> 
> Conceptually it belongs there. It describes a device, not a driver.
> I would say that the name is not well chosen, but it is the right place.
> 
> > usb_driver would be more appropriate.  The drivers that need this have
> > only one entry in their match table, which means that drivers with large
> 
> Why would that be the case? As far as I can see everything that is not
> a class driver will need this or an equivalent and many of them
> support multiple types of devices.

In fact, I'm not sure where to find examples of drivers needing this.  
Apparently only one in drivers/usb/misc/ could benefit (uss720).  The 
other already use usb_find_common_endpoints() or related functions.  
Some of the drivers in atm/ also could use some work.  But there must be 
plenty of others; I just don't know where to look.

The point about how many different devices a driver supports is 
irrelevant; what matters is how it checks the endpoints it will use.  If 
a driver assumes that all the devices it supports will have three 
bulk-OUT endpoints numbered 1, 2, and 3 then it doesn't need a separate 
entry for each usb_device_id in its match table.

> > match tables (which would require a lot of extra space for the new
> > pointers) don't need it.
> 
> A few dozen bytes.

Ho, ho.  Look at usb-storage or uas and think again: two useless 8-byte 
pointers added to each and every entry in the unusual_devs tables.

> > True, the checks could be centralized in usb_probe_interface().  What do
> > you think about doing it that way?
> 
> That really is the best place to put the code for checking.
> And you might put into a comment that the way USB works the endpoint
> number includes the direction. It is not obvious to a casual reader.

The patches already contain such comments, in the patch description and 
in the kerneldoc.

Alan Stern

Re: [syzbot] WARNING in shark_write_reg/usb_submit_urb

[Alan Stern]

The patch has been revised.  Make sure it still works right.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2

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,82 @@ 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.
+ */
+static const struct usb_host_endpoint *usb_find_endpoint(
+		const struct usb_interface *intf, unsigned int ep_addr)
+{
+	int n;
+	const 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;
+}
+
+/**
+ * usb_check_bulk_endpoints - Check whether an interface's current altsetting
+ * contains a set of bulk endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are bulk, %false otherwise.
+ */
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_bulk(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_bulk_endpoints);
+
+/**
+ * usb_check_int_endpoints - Check whether an interface's current altsetting
+ * contains a set of interrupt endpoints with the given addresses.
+ * @intf: the interface whose current altsetting should be searched
+ * @ep_addrs: 0-terminated array of the endpoint addresses (number and
+ * direction) to look for
+ *
+ * Search for endpoints with the specified addresses and check their types.
+ *
+ * Return: %true if all the endpoints are found and are interrupt,
+ * %false otherwise.
+ */
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs)
+{
+	const struct usb_host_endpoint *ep;
+
+	for (; *ep_addrs; ++ep_addrs) {
+		ep = usb_find_endpoint(intf, *ep_addrs);
+		if (!ep || !usb_endpoint_xfer_int(&ep->desc))
+			return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(usb_check_int_endpoints);
+
+/**
  * 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,11 @@ void usb_put_intf(struct usb_interface *
 #define USB_MAXINTERFACES	32
 #define USB_MAXIADS		(USB_MAXINTERFACES/2)
 
+bool usb_check_bulk_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+bool usb_check_int_endpoints(
+		const struct usb_interface *intf, const u8 *ep_addrs);
+
 /*
  * USB Resume Timer: Every Host controller driver should drive the resume
  * signalling on the bus for the amount of time defined by this macro.


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
@@ -316,6 +316,16 @@ static int usb_shark_probe(struct usb_in
 {
 	struct shark_device *shark;
 	int retval = -ENOMEM;
+	static const u8 ep_addresses[] = {
+		SHARK_IN_EP | USB_DIR_IN,
+		SHARK_OUT_EP | USB_DIR_OUT,
+		0};
+
+	/* Are the expected endpoints present? */
+	if (!usb_check_int_endpoints(intf, ep_addresses)) {
+		dev_err(&intf->dev, "Invalid radioSHARK device\n");
+		return -EINVAL;
+	}
 
 	shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
 	if (!shark)
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
@@ -282,6 +282,16 @@ static int usb_shark_probe(struct usb_in
 {
 	struct shark_device *shark;
 	int retval = -ENOMEM;
+	static const u8 ep_addresses[] = {
+		SHARK_IN_EP | USB_DIR_IN,
+		SHARK_OUT_EP | USB_DIR_OUT,
+		0};
+
+	/* Are the expected endpoints present? */
+	if (!usb_check_int_endpoints(intf, ep_addresses)) {
+		dev_err(&intf->dev, "Invalid radioSHARK2 device\n");
+		return -EINVAL;
+	}
 
 	shark = kzalloc(sizeof(struct shark_device), GFP_KERNEL);
 	if (!shark)

Re: [syzbot] [usb?] WARNING in shark_write_reg/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+4b3f8190f6e13b3efd74@syzkaller.appspotmail.com

Tested on:

commit:         c9c3395d Linux 6.2
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.2
console output: https://syzkaller.appspot.com/x/log.txt?x=12bcc3abc80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=fea01b13d861cd1e
dashboard link: https://syzkaller.appspot.com/bug?extid=4b3f8190f6e13b3efd74
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=17f2c727c80000

Note: testing is done by a robot and is best-effort only.