Re: [PATCH v2] char: xillybus: Check endpoint type before allocing

[Greg KH]

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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > ---
> > > > > 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