lkml.org 
[lkml]   [2022]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2022-05-14 14:54    [W:3.481 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site