Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <Yn9XwHxWsLIJXlHu@kroah.com> <20220514114819.2773691-1-zheyuma97@gmail.com>
 <Yn+va5fTsuaFTxVR@kroah.com> <CAMhUBj=RMJwn2K+rQC9rQ=QEe5QkiJ29rMd8KzEC8B7vtXo+ug@mail.gmail.com>
 <Yocp+WZ0On9/wObu@kroah.com>
In-Reply-To: <Yocp+WZ0On9/wObu@kroah.com>
From:   Zheyu Ma <zheyuma97@gmail.com>
Date:   Sun, 22 May 2022 13:06:59 +0800
Message-ID: <CAMhUBj=J5-V_D_kucpiWz6ZdOSSR1N9nRXQxOKgmwRA+LxW17Q@mail.gmail.com>
Subject: Re: [PATCH v2] char: xillybus: Check endpoint type before allocing
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     eli.billauer@gmail.com, arnd@arndb.de,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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