While vmk80xx does have endpoint checking implemented, some things
can fall through the cracks. Depending on the hardware model,
URBs can have either bulk or interrupt type, and current version
of vmk80xx_find_usb_endpoints() function does not take that fully
into account. While this warning does not seem to be too harmful,
at the very least it will crash systems with 'panic_on_warn' set on
them.
Fix the issue found by Syzkaller [1] by somewhat simplifying the
endpoint checking process with usb_find_common_endpoints() and
ensuring that only expected endpoint types are present.
This patch has not been tested on real hardware.
[1] Syzkaller report:
usb 1-1: BOGUS urb xfer, pipe 1 != type 3
WARNING: CPU: 0 PID: 781 at drivers/usb/core/urb.c:504 usb_submit_urb+0xc4e/0x18c0 drivers/usb/core/urb.c:503
..
Call Trace:
<TASK>
usb_start_wait_urb+0x113/0x520 drivers/usb/core/message.c:59
vmk80xx_reset_device drivers/comedi/drivers/vmk80xx.c:227 [inline]
vmk80xx_auto_attach+0xa1c/0x1a40 drivers/comedi/drivers/vmk80xx.c:818
comedi_auto_config+0x238/0x380 drivers/comedi/drivers.c:1067
usb_probe_interface+0x5cd/0xb00 drivers/usb/core/driver.c:399
..
Similar issue also found by Syzkaller:
Link: https://syzkaller.appspot.com/bug?extid=5205eb2f17de3e01946e
Reported-and-tested-by: [email protected]
Fixes: 49253d542cc0 ("staging: comedi: vmk80xx: factor out usb endpoint detection")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
drivers/comedi/drivers/vmk80xx.c | 35 ++++++++++++-----------------------
1 file changed, 12 insertions(+), 23 deletions(-)
diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
index 4536ed43f65b..476885403c61 100644
--- a/drivers/comedi/drivers/vmk80xx.c
+++ b/drivers/comedi/drivers/vmk80xx.c
@@ -641,33 +641,22 @@ static int vmk80xx_find_usb_endpoints(struct comedi_device *dev)
struct vmk80xx_private *devpriv = dev->private;
struct usb_interface *intf = comedi_to_usb_interface(dev);
struct usb_host_interface *iface_desc = intf->cur_altsetting;
- struct usb_endpoint_descriptor *ep_desc;
- int i;
+ struct usb_endpoint_descriptor *ep_rx_desc, *ep_tx_desc;
+ int i, ret;
- if (iface_desc->desc.bNumEndpoints != 2)
- return -ENODEV;
-
- for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
- ep_desc = &iface_desc->endpoint[i].desc;
-
- if (usb_endpoint_is_int_in(ep_desc) ||
- usb_endpoint_is_bulk_in(ep_desc)) {
- if (!devpriv->ep_rx)
- devpriv->ep_rx = ep_desc;
- continue;
- }
-
- if (usb_endpoint_is_int_out(ep_desc) ||
- usb_endpoint_is_bulk_out(ep_desc)) {
- if (!devpriv->ep_tx)
- devpriv->ep_tx = ep_desc;
- continue;
- }
- }
+ if (devpriv->model == VMK8061_MODEL)
+ ret = usb_find_common_endpoints(iface_desc, &ep_rx_desc,
+ &ep_tx_desc, NULL, NULL);
+ else
+ ret = usb_find_common_endpoints(iface_desc, NULL, NULL,
+ &ep_rx_desc, &ep_tx_desc);
- if (!devpriv->ep_rx || !devpriv->ep_tx)
+ if (ret)
return -ENODEV;
+ devpriv->ep_rx = ep_rx_desc;
+ devpriv->ep_tx = ep_tx_desc;
+
if (!usb_endpoint_maxp(devpriv->ep_rx) || !usb_endpoint_maxp(devpriv->ep_tx))
return -EINVAL;
On 07/04/2024 17:26, Nikita Zhandarovich wrote:
> While vmk80xx does have endpoint checking implemented, some things
> can fall through the cracks. Depending on the hardware model,
> URBs can have either bulk or interrupt type, and current version
> of vmk80xx_find_usb_endpoints() function does not take that fully
> into account. While this warning does not seem to be too harmful,
> at the very least it will crash systems with 'panic_on_warn' set on
> them.
>
> Fix the issue found by Syzkaller [1] by somewhat simplifying the
> endpoint checking process with usb_find_common_endpoints() and
> ensuring that only expected endpoint types are present.
>
> This patch has not been tested on real hardware.
>
> [1] Syzkaller report:
> usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> WARNING: CPU: 0 PID: 781 at drivers/usb/core/urb.c:504 usb_submit_urb+0xc4e/0x18c0 drivers/usb/core/urb.c:503
> ...
> Call Trace:
> <TASK>
> usb_start_wait_urb+0x113/0x520 drivers/usb/core/message.c:59
> vmk80xx_reset_device drivers/comedi/drivers/vmk80xx.c:227 [inline]
> vmk80xx_auto_attach+0xa1c/0x1a40 drivers/comedi/drivers/vmk80xx.c:818
> comedi_auto_config+0x238/0x380 drivers/comedi/drivers.c:1067
> usb_probe_interface+0x5cd/0xb00 drivers/usb/core/driver.c:399
> ...
>
> Similar issue also found by Syzkaller:
> Link: https://syzkaller.appspot.com/bug?extid=5205eb2f17de3e01946e
>
> Reported-and-tested-by: [email protected]
> Fixes: 49253d542cc0 ("staging: comedi: vmk80xx: factor out usb endpoint detection")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> drivers/comedi/drivers/vmk80xx.c | 35 ++++++++++++-----------------------
> 1 file changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
> index 4536ed43f65b..476885403c61 100644
> --- a/drivers/comedi/drivers/vmk80xx.c
> +++ b/drivers/comedi/drivers/vmk80xx.c
> @@ -641,33 +641,22 @@ static int vmk80xx_find_usb_endpoints(struct comedi_device *dev)
> struct vmk80xx_private *devpriv = dev->private;
> struct usb_interface *intf = comedi_to_usb_interface(dev);
> struct usb_host_interface *iface_desc = intf->cur_altsetting;
> - struct usb_endpoint_descriptor *ep_desc;
> - int i;
> + struct usb_endpoint_descriptor *ep_rx_desc, *ep_tx_desc;
> + int i, ret;
I get a "warning: unused variable 'i' [-Wunused-variable]" warning here.
>
> - if (iface_desc->desc.bNumEndpoints != 2)
> - return -ENODEV;
> -
> - for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
> - ep_desc = &iface_desc->endpoint[i].desc;
> -
> - if (usb_endpoint_is_int_in(ep_desc) ||
> - usb_endpoint_is_bulk_in(ep_desc)) {
> - if (!devpriv->ep_rx)
> - devpriv->ep_rx = ep_desc;
> - continue;
> - }
> -
> - if (usb_endpoint_is_int_out(ep_desc) ||
> - usb_endpoint_is_bulk_out(ep_desc)) {
> - if (!devpriv->ep_tx)
> - devpriv->ep_tx = ep_desc;
> - continue;
> - }
> - }
> + if (devpriv->model == VMK8061_MODEL)
> + ret = usb_find_common_endpoints(iface_desc, &ep_rx_desc,
> + &ep_tx_desc, NULL, NULL);
> + else
> + ret = usb_find_common_endpoints(iface_desc, NULL, NULL,
> + &ep_rx_desc, &ep_tx_desc);
>
> - if (!devpriv->ep_rx || !devpriv->ep_tx)
> + if (ret)
> return -ENODEV;
>
> + devpriv->ep_rx = ep_rx_desc;
> + devpriv->ep_tx = ep_tx_desc;
> +
> if (!usb_endpoint_maxp(devpriv->ep_rx) || !usb_endpoint_maxp(devpriv->ep_tx))
> return -EINVAL;
>
I've tested it on a K8055/VM110 board and it still works OK. I don't
have a K8061/VM140 to test it with, but it should be OK.
Feel free to add "Reviewed-by: Ian Abbott <[email protected]>" after
fixing the compiler warning.
--
-=( Ian Abbott <[email protected]> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || http://www.mev.co.uk )=-