lkml.org 
[lkml]   [2022]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] char: xillybus: Check endpoint type before allocing
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

\
 
 \ /
  Last update: 2022-05-22 07:07    [W:0.231 / U:0.964 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site