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

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

[Zheyu Ma]

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 <zheyuma97@gmail.com>
---
Changes in v3:
    - Check the endpoint type more earlier
Changes in v2:
    - Check the endpoint type at probe time
---
 drivers/char/xillybus/xillyusb.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index dc3551796e5e..4ed19a2a04e3 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 */
@@ -1891,6 +1892,17 @@ static const struct file_operations xillyusb_fops = {
 
 static int xillyusb_setup_base_eps(struct xillyusb_dev *xdev)
 {
+	int ret;
+	struct usb_endpoint_descriptor *in, *out;
+
+	ret = usb_find_common_endpoints(xdev->intf->cur_altsetting, &in, &out, NULL, NULL);
+	if (ret)
+		return ret;
+
+	if (in->bEndpointAddress != (IN_EP_NUM | USB_DIR_IN) ||
+		out->bEndpointAddress != (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)
@@ -1916,6 +1928,21 @@ static int xillyusb_setup_base_eps(struct xillyusb_dev *xdev)
 	return -ENOMEM;
 }
 
+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 && usb_endpoint_is_bulk_out(ep))
+			return 0;
+	}
+
+	return -EINVAL;
+}
+
 static int setup_channels(struct xillyusb_dev *xdev,
 			  __le16 *chandesc,
 			  int num_channels)
@@ -1962,6 +1989,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 +2156,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

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

[Eli Billauer]

On 29/05/22 09:58, Zheyu Ma wrote:
>   static int xillyusb_setup_base_eps(struct xillyusb_dev *xdev)
>   {
> +	int ret;
> +	struct usb_endpoint_descriptor *in, *out;
> +
> +	ret = usb_find_common_endpoints(xdev->intf->cur_altsetting,&in,&out, NULL, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (in->bEndpointAddress != (IN_EP_NUM | USB_DIR_IN) ||
> +		out->bEndpointAddress != (MSG_EP_NUM | USB_DIR_OUT))
> +		return -EINVAL;
> +
>    
As far as I understand, this finds the first BULK endpoints in both 
directions, and verifies that their addresses are MSG_EP_NUM and 
IN_EP_NUM. Because both of these happen to equal 1, I suppose this 
indeed checks the right thing. But am I right that this won't work if 
either MSG_EP_NUM or IN_EP_NUM have a value that isn't 1? Not that I 
think that will ever happen, but still.

> +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&&  usb_endpoint_is_bulk_out(ep))
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>
>    
Given that you've added this function, why isn't it used in 
xillyusb_setup_base_eps()?

Thanks,
    Eli

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

[Zheyu Ma]

On Mon, May 30, 2022 at 11:13 PM Eli Billauer <eli.billauer@gmail.com> wrote:
>
> On 29/05/22 09:58, Zheyu Ma wrote:
> >   static int xillyusb_setup_base_eps(struct xillyusb_dev *xdev)
> >   {
> > +     int ret;
> > +     struct usb_endpoint_descriptor *in, *out;
> > +
> > +     ret = usb_find_common_endpoints(xdev->intf->cur_altsetting,&in,&out, NULL, NULL);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (in->bEndpointAddress != (IN_EP_NUM | USB_DIR_IN) ||
> > +             out->bEndpointAddress != (MSG_EP_NUM | USB_DIR_OUT))
> > +             return -EINVAL;
> > +
> >
> As far as I understand, this finds the first BULK endpoints in both
> directions, and verifies that their addresses are MSG_EP_NUM and
> IN_EP_NUM. Because both of these happen to equal 1, I suppose this
> indeed checks the right thing. But am I right that this won't work if
> either MSG_EP_NUM or IN_EP_NUM have a value that isn't 1? Not that I
> think that will ever happen, but still.

Indeed, the correctness of this code comes from the fact that both
MSG_EP_NUM and IN_EP_NUM are the first bulk endpoint, without such an
assumption this check does not hold.
I did this just for convenience, we can also use
xillyusb_check_endpoint() to check them.

> > +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&&  usb_endpoint_is_bulk_out(ep))
> > +                     return 0;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> >
> >
> Given that you've added this function, why isn't it used in
> xillyusb_setup_base_eps()?

It is feasible to use this function for checking in
xillyusb_setup_base_eps(), perhaps with some modifications. Anyway, I
will do so in the next version of the patch.

Thanks,
Zheyu Ma