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 [email protected].
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);
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
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.
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;
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
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.
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 <[email protected]>
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;
>
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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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/[email protected]
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.
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
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
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
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
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 <[email protected]>
> > >
> > > 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
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)
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);
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
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.
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
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.