Messages in this thread | | | Date | Sat, 14 May 2022 15:52:20 +0300 | From | Eli Billauer <> | Subject | Re: [PATCH v2] char: xillybus: Check endpoint type before allocing |
| |
On 14/05/22 14:48, Zheyu Ma wrote: > The driver submits bulk urb without checking the endpoint type is > actually bulk. > > [ 3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > [ 3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0 > [ 3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0 > [ 3.115318] Call Trace: > [ 3.115452]<TASK> > [ 3.115570] try_queue_bulk_in+0x43c/0x6e0 [xillyusb] > [ 3.115838] xillyusb_probe+0x488/0x1230 [xillyusb] > > Add a check in endpoint_alloc() to fix the bug. > > Signed-off-by: Zheyu Ma<zheyuma97@gmail.com> > --- > Changes in v2: > - Check the endpoint type at probe time > --- > drivers/char/xillybus/xillyusb.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c > index dc3551796e5e..4467f13993ef 100644 > --- a/drivers/char/xillybus/xillyusb.c > +++ b/drivers/char/xillybus/xillyusb.c > @@ -167,6 +167,7 @@ struct xillyusb_dev { > struct device *dev; /* For dev_err() and such */ > struct kref kref; > struct workqueue_struct *workq; > + struct usb_interface *intf; > > int error; > spinlock_t error_lock; /* protect @error */ > @@ -475,6 +476,25 @@ static void endpoint_dealloc(struct xillyusb_endpoint *ep) > kfree(ep); > } > > +static int xillyusb_check_endpoint(struct xillyusb_dev *xdev, u8 ep_num) > +{ > + struct usb_host_interface *if_desc = xdev->intf->altsetting; > + int i; > + > + for (i = 0; i< if_desc->desc.bNumEndpoints; i++) { > + struct usb_endpoint_descriptor *ep =&if_desc->endpoint[i].desc; > + > + if (ep->bEndpointAddress != ep_num) > + continue; > + > + if ((usb_pipein(ep_num)&& usb_endpoint_is_bulk_in(ep)) || > + (usb_pipeout(ep_num)&& usb_endpoint_is_bulk_out(ep))) > + return 0; > + } > + > + return -EINVAL; > +} > + > static struct xillyusb_endpoint > *endpoint_alloc(struct xillyusb_dev *xdev, > u8 ep_num, > @@ -482,10 +502,14 @@ static struct xillyusb_endpoint > unsigned int order, > int bufnum) > { > - int i; > + int i, ret; > > struct xillyusb_endpoint *ep; > > + ret = xillyusb_check_endpoint(xdev, ep_num); > + if (ret) > + return NULL; > + > ep = kzalloc(sizeof(*ep), GFP_KERNEL); > > if (!ep) > @@ -2125,6 +2149,7 @@ static int xillyusb_probe(struct usb_interface *interface, > mutex_init(&xdev->process_in_mutex); > mutex_init(&xdev->msg_mutex); > > + xdev->intf = interface; > xdev->udev = usb_get_dev(interface_to_usbdev(interface)); > xdev->dev =&interface->dev; > xdev->error = 0; > I wonder why this check is necessary. The XillyUSB device presents bulk endpoints only, and the driver never tries anything else.
Actually, if this warning appears in a real-life scenario, it definitely indicates a serious problem. So I think the warning should be visible, rather than silenced like this.
I realize that a code sanitizer has been triggered, but is there more to this?
Thanks, Eli
| |