Messages in this thread | | | Date | Mon, 8 Apr 2024 16:25:39 +0100 | Subject | Re: [PATCH] comedi: vmk80xx: fix incomplete endpoint checking | From | Ian Abbott <> |
| |
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: syzbot+5f29dc6a889fc42bd896@syzkaller.appspotmail.com > Fixes: 49253d542cc0 ("staging: comedi: vmk80xx: factor out usb endpoint detection") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > 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 <abbotti@mev.co.uk>" after fixing the compiler warning.
-- -=( Ian Abbott <abbotti@mev.co.uk> || 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. || www.mev.co.uk )=-
| |