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<[email protected]>
> ---
> 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
On Sat, May 14, 2022 at 03:52:20PM +0300, Eli Billauer wrote:
> 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<[email protected]>
> > ---
> > 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.
And the driver needs to check this to verify it. Think about a "fake"
device, you have to catch that.
thanks,
greg k-h