Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Yn9XwHxWsLIJXlHu@kroah.com> <20220514114819.2773691-1-zheyuma97@gmail.com>
 <Yn+va5fTsuaFTxVR@kroah.com>
In-Reply-To: <Yn+va5fTsuaFTxVR@kroah.com>
From:   Zheyu Ma <zheyuma97@gmail.com>
Date:   Fri, 20 May 2022 11:32:51 +0800
Message-ID: <CAMhUBj=RMJwn2K+rQC9rQ=QEe5QkiJ29rMd8KzEC8B7vtXo+ug@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 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