[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <627FA604.2010302@gmail.com>
Date: Sat, 14 May 2022 15:52:20 +0300
From: Eli Billauer <eli.billauer@...il.com>
To: Zheyu Ma <zheyuma97@...il.com>
CC: arnd@...db.de, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org
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@...il.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
Powered by blists - more mailing lists