2022-05-31 16:47:21

by Zheyu Ma

Subject: [PATCH v4] char: xillybus: Check endpoint type at probe time

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 at probe time to fix the bug.

Signed-off-by: Zheyu Ma <[email protected]>
---
Changes in v4:
- Use function xillyusb_check_endpoint() to check the endpoint in
xillyusb_setup_base_eps()
Changes in v3:
- Check the endpoint type more earlier
Changes in v2:
- Check the endpoint type at probe time
---
drivers/char/xillybus/xillyusb.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index dc3551796e5e..596964e65891 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 */
@@ -1889,8 +1890,31 @@ static const struct file_operations xillyusb_fops = {
.poll = xillyusb_poll,
};

+static int xillyusb_check_endpoint(struct xillyusb_dev *xdev, u8 addr)
+{
+ int i;
+ struct usb_host_interface *if_desc = xdev->intf->altsetting;
+
+ for (i = 0; i < if_desc->desc.bNumEndpoints; i++) {
+ struct usb_endpoint_descriptor *ep = &if_desc->endpoint[i].desc;
+
+ if (ep->bEndpointAddress != addr)
+ continue;
+
+ if ((usb_pipein(addr) && usb_endpoint_is_bulk_in(ep)) ||
+ (usb_pipeout(addr) && usb_endpoint_is_bulk_out(ep)))
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
static int xillyusb_setup_base_eps(struct xillyusb_dev *xdev)
{
+ if (xillyusb_check_endpoint(xdev, IN_EP_NUM | USB_DIR_IN) ||
+ xillyusb_check_endpoint(xdev, MSG_EP_NUM | USB_DIR_OUT))
+ return -EINVAL;
+
xdev->msg_ep = endpoint_alloc(xdev, MSG_EP_NUM | USB_DIR_OUT,
bulk_out_work, 1, 2);
if (!xdev->msg_ep)
@@ -1962,6 +1986,10 @@ static int setup_channels(struct xillyusb_dev *xdev,
chan->out_log2_element_size = out_desc & 0x0f;
chan->out_log2_fifo_size =
((out_desc >> 8) & 0x1f) + 16;
+ if (xillyusb_check_endpoint(xdev, (i+2) | USB_DIR_OUT)) {
+ kfree(xdev->channels);
+ return -EINVAL;
+ }
}
}

@@ -2125,6 +2153,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;
--
2.25.1

2022-06-06 09:51:04

by Eli Billauer

Subject: Re: [PATCH v4] char: xillybus: Check endpoint type at probe time

On 31/05/22 08:44, Zheyu Ma wrote:
> static int xillyusb_setup_base_eps(struct xillyusb_dev *xdev)
> {
> + if (xillyusb_check_endpoint(xdev, IN_EP_NUM | USB_DIR_IN) ||
> + xillyusb_check_endpoint(xdev, MSG_EP_NUM | USB_DIR_OUT))
> + return -EINVAL;
> +
> xdev->msg_ep = endpoint_alloc(xdev, MSG_EP_NUM | USB_DIR_OUT,
> bulk_out_work, 1, 2);
> if (!xdev->msg_ep)
> @@ -1962,6 +1986,10 @@ static int setup_channels(struct xillyusb_dev *xdev,
> chan->out_log2_element_size = out_desc& 0x0f;
> chan->out_log2_fifo_size =
> ((out_desc>> 8)& 0x1f) + 16;
> + if (xillyusb_check_endpoint(xdev, (i+2) | USB_DIR_OUT)) {
> + kfree(xdev->channels);
> + return -EINVAL;
> + }
> }
> }
>
>
I just checked this against hardware, and made it fail deliberately by
replacing the (i+2) part with (i+3). I got a kernel memory corruption,
because xdev->channels was freed twice: Once in the snippet above, and a
second time in cleanup_dev().

So the kfree() call here should go away. xdev->channels is freed when
xdev's reference count goes to zero.

Thanks,
Eli