The driver submits bulk urb without checking the endpoint type is
actually bulk.

[    3.108411] ------------[ cut here ]------------
[    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 to fix it.

Fixes: a53d1202aef1 ("char: xillybus: Add driver for XillyUSB (Xillybus variant for USB)")
Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 drivers/char/xillybus/xillyusb.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 39bcbfd908b4..2ec2e087b2e7 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -730,6 +730,11 @@ static void try_queue_bulk_in(struct xillyusb_endpoint *ep)
 
 		usb_anchor_urb(urb, &ep->anchor);
 
+		if (usb_urb_ep_type_check(urb)) {
+			report_io_error(xdev, -EINVAL);
+			goto unanchor;
+		}
+
 		rc = usb_submit_urb(urb, GFP_KERNEL);
 
 		if (rc) {
@@ -834,6 +839,11 @@ static void try_queue_bulk_out(struct xillyusb_endpoint *ep)
 
 		usb_anchor_urb(urb, &ep->anchor);
 
+		if (usb_urb_ep_type_check(urb)) {
+			report_io_error(xdev, -EINVAL);
+			goto unanchor;
+		}
+
 		rc = usb_submit_urb(urb, GFP_KERNEL);
 
 		if (rc) {
-- 
2.25.1

On Sat, May 14, 2022 at 03:14:35PM +0800, Zheyu Ma wrote:
> The driver submits bulk urb without checking the endpoint type is
> actually bulk.
> 
> [    3.108411] ------------[ cut here ]------------
> [    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 to fix it.
> 
> Fixes: a53d1202aef1 ("char: xillybus: Add driver for XillyUSB (Xillybus variant for USB)")
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  drivers/char/xillybus/xillyusb.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
> index 39bcbfd908b4..2ec2e087b2e7 100644
> --- a/drivers/char/xillybus/xillyusb.c
> +++ b/drivers/char/xillybus/xillyusb.c
> @@ -730,6 +730,11 @@ static void try_queue_bulk_in(struct xillyusb_endpoint *ep)
>  
>  		usb_anchor_urb(urb, &ep->anchor);
>  
> +		if (usb_urb_ep_type_check(urb)) {
> +			report_io_error(xdev, -EINVAL);
> +			goto unanchor;
> +		}
> +
>  		rc = usb_submit_urb(urb, GFP_KERNEL);
>  
>  		if (rc) {
> @@ -834,6 +839,11 @@ static void try_queue_bulk_out(struct xillyusb_endpoint *ep)
>  
>  		usb_anchor_urb(urb, &ep->anchor);
>  
> +		if (usb_urb_ep_type_check(urb)) {
> +			report_io_error(xdev, -EINVAL);
> +			goto unanchor;
> +		}
> +
>  		rc = usb_submit_urb(urb, GFP_KERNEL);
>  
>  		if (rc) {
> -- 
> 2.25.1
> 

This should all be checked much earlier, in your probe function, to make
sure you have the endpoints correct.  Just refuse to bind to a device
that is not correct.  Don't check late here when you are submitting the
urb.

thanks,

greg k-h

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;
-- 
2.25.1

On Sat, May 14, 2022 at 07:48:19PM +0800, 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;
> +	}

Why not use the built-in usb core functions that do this for you instead
of hand-parsing this?  Look at usb_find_common_endpoints() and related
functions, that should make this much easier.

thanks,

greg k-h

On Sat, May 14, 2022 at 9:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, May 14, 2022 at 07:48:19PM +0800, 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;
> > +     }
>
> Why not use the built-in usb core functions that do this for you instead
> of hand-parsing this?  Look at usb_find_common_endpoints() and related
> functions, that should make this much easier.

Thanks for your reminder. But in this driver, we have to check not
only the type and direction of the endpoint, but also the address of
it. And the endpoint's address is sometimes dynamic. For example,  in
the function xillyusb_open():

out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
bulk_out_work, BUF_SIZE_ORDER, BUFNUM);

However, usb_find_common_endpoints() can only find the first endpoint
that satisfies the condition, not on a specific address. I cannot find
a more suitable built-in core function, please correct me if I'm
wrong.

Thanks,
Zheyu Ma

On Fri, May 20, 2022 at 11:32:51AM +0800, Zheyu Ma wrote:
> On Sat, May 14, 2022 at 9:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, May 14, 2022 at 07:48:19PM +0800, 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;
> > > +     }
> >
> > Why not use the built-in usb core functions that do this for you instead
> > of hand-parsing this?  Look at usb_find_common_endpoints() and related
> > functions, that should make this much easier.
> 
> Thanks for your reminder. But in this driver, we have to check not
> only the type and direction of the endpoint, but also the address of
> it. And the endpoint's address is sometimes dynamic. For example,  in
> the function xillyusb_open():
> 
> out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
> bulk_out_work, BUF_SIZE_ORDER, BUFNUM);
> 
> However, usb_find_common_endpoints() can only find the first endpoint
> that satisfies the condition, not on a specific address. I cannot find
> a more suitable built-in core function, please correct me if I'm
> wrong.

I do not understand the problem here, it looks like your code above that
I responded to doesn't care about specific addresses at all.  It is just
walking all of them and making sure that it is a bulk in/out endpoint.

And why do you care about the address of the endpoint?  If you know
that, then there's no need to walk them all and you can check that even
easier.

thanks,

greg k-h

On Fri, May 20, 2022 at 1:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 20, 2022 at 11:32:51AM +0800, Zheyu Ma wrote:
> > On Sat, May 14, 2022 at 9:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sat, May 14, 2022 at 07:48:19PM +0800, 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;
> > > > +     }
> > >
> > > Why not use the built-in usb core functions that do this for you instead
> > > of hand-parsing this?  Look at usb_find_common_endpoints() and related
> > > functions, that should make this much easier.
> >
> > Thanks for your reminder. But in this driver, we have to check not
> > only the type and direction of the endpoint, but also the address of
> > it. And the endpoint's address is sometimes dynamic. For example,  in
> > the function xillyusb_open():
> >
> > out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
> > bulk_out_work, BUF_SIZE_ORDER, BUFNUM);
> >
> > However, usb_find_common_endpoints() can only find the first endpoint
> > that satisfies the condition, not on a specific address. I cannot find
> > a more suitable built-in core function, please correct me if I'm
> > wrong.
>
> I do not understand the problem here, it looks like your code above that
> I responded to doesn't care about specific addresses at all.  It is just
> walking all of them and making sure that it is a bulk in/out endpoint.

Please correct me if I'm wrong. I think the driver needs to check if
the urb has the correct type before submitting the urb, and this check
should be done early.
This driver uses endpoint_alloc() to allocate an endpoint and then
uses this endpoint for transfers. And one of the arguments of
endpoint_alloc() is 'ep_num'. So I want to iterate over the endpoints
and exclude the unwanted address as follows:

 if (ep->bEndpointAddress != ep_num)
    continue;

> And why do you care about the address of the endpoint?  If you know
> that, then there's no need to walk them all and you can check that even
> easier.

I care about the address of the endpoint because it is an argument of
endpoint_alloc(). I don't know a better solution to check the endpoint
even if I know the address.

Thanks,
Zheyu Ma

On Sun, May 22, 2022 at 01:06:59PM +0800, Zheyu Ma wrote:
> On Fri, May 20, 2022 at 1:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, May 20, 2022 at 11:32:51AM +0800, Zheyu Ma wrote:
> > > On Sat, May 14, 2022 at 9:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sat, May 14, 2022 at 07:48:19PM +0800, 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;
> > > > > +     }
> > > >
> > > > Why not use the built-in usb core functions that do this for you instead
> > > > of hand-parsing this?  Look at usb_find_common_endpoints() and related
> > > > functions, that should make this much easier.
> > >
> > > Thanks for your reminder. But in this driver, we have to check not
> > > only the type and direction of the endpoint, but also the address of
> > > it. And the endpoint's address is sometimes dynamic. For example,  in
> > > the function xillyusb_open():
> > >
> > > out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
> > > bulk_out_work, BUF_SIZE_ORDER, BUFNUM);
> > >
> > > However, usb_find_common_endpoints() can only find the first endpoint
> > > that satisfies the condition, not on a specific address. I cannot find
> > > a more suitable built-in core function, please correct me if I'm
> > > wrong.
> >
> > I do not understand the problem here, it looks like your code above that
> > I responded to doesn't care about specific addresses at all.  It is just
> > walking all of them and making sure that it is a bulk in/out endpoint.
> 
> Please correct me if I'm wrong. I think the driver needs to check if
> the urb has the correct type before submitting the urb, and this check
> should be done early.

Yes, very very early, like probe() callback time early.

Not way down here in "do we want to allow open() to work or not" like
you are currently doing.

If the device does not have the EXACT USB endpoints that you are
expecting (size, address, direction, type, etc.) at probe time reject
the device.

That is what the helper functions I pointed you at are for.

This driver is trying to detect this type of problem _way_ too late.

thanks,

greg k-h

On 23/05/22 19:06, Greg KH wrote:
> If the device does not have the EXACT USB endpoints that you are
> expecting (size, address, direction, type, etc.) at probe time reject
> the device.
>    
This is probably a good time to add some information about XillyUSB.

All XillyUSB devices have EP 1 IN and EP 1 OUT as bulk endpoints.

On top of that, they *may* have up to 14 additional bulk OUT endpoints, 
having the addresses EP 2 OUT to EP 15 OUT. The population of endpoint 
addresses is not necessarily continuous. Any set of OUT endpoint 
addresses is allowed. The driver doesn't know which of these endpoints 
are available initially.

Rather, it works like this: The driver uses the EP 1 IN and OUT 
endpoints to query the device for a data structure, which contains 
information on the device's communication channels. The driver sets up 
device files accordingly, and it also gets informed on which bulk OUT 
endpoints are present.

For what it's worth, I think it's fairly safe to assume that if a device 
returns a legal data structure (which passes a CRC test), it's a 
XillyUSB device. Either way, it's impossible to verify that all of the 
device's bulk OUT endpoints are correct before obtaining the data 
structure from the device. The fact that each device has a different set 
of communication channels, and that the driver learns about them in 
run-time is the whole trick with Xillybus and XillyUSB.

And just in case you wonder why there's only one bulk IN endpoint: All 
inbound communication, control as well as data, is multiplexed into this 
single endpoint. That's in order to allow the device better control on 
which communication channel is serviced at any time, with a few 
microseconds' granularity. The same trick is unfortunately infeasible in 
the other direction.

I don't have any particular view on how the device should be validated, 
but I thought this information would be helpful.

Thanks,
    Eli

On Mon, May 23, 2022 at 08:05:47PM +0300, Eli Billauer wrote:
> On 23/05/22 19:06, Greg KH wrote:
> > If the device does not have the EXACT USB endpoints that you are
> > expecting (size, address, direction, type, etc.) at probe time reject
> > the device.
> This is probably a good time to add some information about XillyUSB.
> 
> All XillyUSB devices have EP 1 IN and EP 1 OUT as bulk endpoints.
> 
> On top of that, they *may* have up to 14 additional bulk OUT endpoints,
> having the addresses EP 2 OUT to EP 15 OUT. The population of endpoint
> addresses is not necessarily continuous. Any set of OUT endpoint addresses
> is allowed. The driver doesn't know which of these endpoints are available
> initially.
> 
> Rather, it works like this: The driver uses the EP 1 IN and OUT endpoints to
> query the device for a data structure, which contains information on the
> device's communication channels. The driver sets up device files
> accordingly, and it also gets informed on which bulk OUT endpoints are
> present.
> 
> For what it's worth, I think it's fairly safe to assume that if a device
> returns a legal data structure (which passes a CRC test), it's a XillyUSB
> device.

Why?  You still need to verify that the requested endpoints match up
with what the device really has.

Again, we are talking about "fake" devices that are being used to find
problems in the kernel drivers, this is not a "real" device, yet can
look a lot like one.  Look at the fuzzing tools that are running for
examples of this.

> Either way, it's impossible to verify that all of the device's bulk
> OUT endpoints are correct before obtaining the data structure from the
> device. The fact that each device has a different set of communication
> channels, and that the driver learns about them in run-time is the whole
> trick with Xillybus and XillyUSB.

That's fine, but that can still be done a probe() time, not open() time,
like the current patch was attempting.  That's much too late.

thanks,

greg k-h

On 23/05/22 20:15, Greg KH wrote:
>> Rather, it works like this: The driver uses the EP 1 IN and OUT endpoints to
>> >  query the device for a data structure, which contains information on the
>> >  device's communication channels. The driver sets up device files
>> >  accordingly, and it also gets informed on which bulk OUT endpoints are
>> >  present.
>> >  
>> >  For what it's worth, I think it's fairly safe to assume that if a device
>> >  returns a legal data structure (which passes a CRC test), it's a XillyUSB
>> >  device.
>>      
> Why?  You still need to verify that the requested endpoints match up
> with what the device really has.
>    
OK. So to summarize:

EP 1 IN and EP 1 OUT are always present in XillyUSB devices.

On top of these, there might be additional bulk OUT endpoints. The 
driver resolves which ones in setup_channels(), by scanning a data blob 
it has received from the device. This takes place in the for-loop inside 
this function. When (out_desc & 0x80) is true for a given @i in the 
loop, the device has a bulk OUT endpoint with address i+2 (e.g. if this 
condition is met for i==2, the device has a bulk OUT EP 4).

So it seems like setup_channels() would be the right place to make this 
check, since it's called during the device's probe.

I guess it would likewise make sense to check for EP 1 IN and OUT in 
xillyusb_setup_base_eps().

Thanks,
    Eli

On Tue, May 24, 2022 at 8:24 PM Eli Billauer <eli.billauer@gmail.com> wrote:
>
> On 23/05/22 20:15, Greg KH wrote:
> >> Rather, it works like this: The driver uses the EP 1 IN and OUT endpoints to
> >> >  query the device for a data structure, which contains information on the
> >> >  device's communication channels. The driver sets up device files
> >> >  accordingly, and it also gets informed on which bulk OUT endpoints are
> >> >  present.
> >> >
> >> >  For what it's worth, I think it's fairly safe to assume that if a device
> >> >  returns a legal data structure (which passes a CRC test), it's a XillyUSB
> >> >  device.
> >>
> > Why?  You still need to verify that the requested endpoints match up
> > with what the device really has.
> >
> OK. So to summarize:
>
> EP 1 IN and EP 1 OUT are always present in XillyUSB devices.
>
> On top of these, there might be additional bulk OUT endpoints. The
> driver resolves which ones in setup_channels(), by scanning a data blob
> it has received from the device. This takes place in the for-loop inside
> this function. When (out_desc & 0x80) is true for a given @i in the
> loop, the device has a bulk OUT endpoint with address i+2 (e.g. if this
> condition is met for i==2, the device has a bulk OUT EP 4).
>
> So it seems like setup_channels() would be the right place to make this
> check, since it's called during the device's probe.
>
> I guess it would likewise make sense to check for EP 1 IN and OUT in
> xillyusb_setup_base_eps().
>
> Thanks,
>     Eli

Thanks for the detailed explanation, I will try to send a new version
of the patch to fix the bug.

Thanks,
Zheyu Ma

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

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<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.

And the driver needs to check this to verify it.  Think about a "fake"
device, you have to catch that.

thanks,

greg k-h