Date:   Thu, 24 Feb 2022 11:46:40 +0100
From:   Cristiano Giuffrida <c.giuffrida@...nl>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Jakob <jakobkoschel@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergman <arnd@...db.de>,
        "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Mike Rapoport <rppt@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        "Brian Johannesmeyer" <bjohannesmeyer@...il.com>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator
 after the loop

I think the "problem" with this solution is that it doesn't prevent
`tmp` from being used outside the loop still (and people getting it
wrong again)? It would be good to have 'struct gr_request *tmp;' being
visible only inside the loop (i.e., declared in the macro).

On Thu, Feb 24, 2022 at 11:34 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Wed, Feb 23, 2022 at 03:16:09PM +0100, Jakob wrote:
> > Note that I changed the location of the struct member 'req' in gr_request
> > to make this work. Instead of reshuffling struct members this can also be
> > introduced by simply adding new struct members in certain spots.
> > In other code locations with the same pattern I didn't have to do that.
> >
> > Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
> > heap, the check could pass even though the list searched is completely
> > empty.
> >
> > &req->req for the head element will be an out-of-bounds pointer
> > that by coincidence or heap massaging is where '_req' is located.
> >
> > Even if the list is empty the list_for_each_entry() macro will do:
> >
> >       pos = list_first_entry(head, typeof(*pos), member);
> >
> > resolving all the macros (list_first_entry() etc) it will look like this:
> >
> >       pos = container_of(head->next, typeof(*pos), member)
> >
> > Since the list is empty head->next == head and container_of() is called on something
> > that is *not* actually of type gr_request.
> >
> > Next, the check if the end of the loop is hit is evaluated:
> >
> >       !list_entry_is_head(pos, head, member);
> >
> > which will stop the loop directly before executing a single iteration.
> >
> > then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
> > where in this case '_req' is located.
> >
> > The point I'm trying to make: it's probably not safe to rely on the compiler and
> > that everyone is aware of this risk when adding/removing/reordering struct members.
> >
> >
> > Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
> > ---
> > drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
> > drivers/usb/gadget/udc/gr_udc.h |  2 +-
> > 2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
> > index 4b35739d3695..29c662f28428 100644
> > --- a/drivers/usb/gadget/udc/gr_udc.c
> > +++ b/drivers/usb/gadget/udc/gr_udc.c
> > @@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> >               ret = -EINVAL;
> >               goto out;
> >       }
> > +     printk(KERN_INFO "JKL: This does print, but shouldn't");
> >
> >       if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
> >               /* This request is currently being processed */
> > @@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> >       return ret;
> > }
> >
> > +static int __init init_test_jkl3(void)
> > +{
> > +     struct gr_ep *ep;
> > +     struct gr_udc *dev;
> > +     struct usb_request *_req;
> > +     void *buffer;
> > +
> > +     /* assume the usb_request struct is located just after the 'ep' on the heap */
> > +     buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
> > +     ep = buffer;
> > +     _req = buffer+sizeof(struct gr_ep);
> > +
> > +     /* setup to call gr_dequeue() */
> > +     dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
> > +     ep->dev = dev;
> > +     ep->dev->driver = 1;
> > +     INIT_LIST_HEAD(&ep->queue); /* list is empty */
> > +
> > +     gr_dequeue(&ep->ep, _req);
> > +
> > +     return 0;
> > +}
> > +__initcall(init_test_jkl3);
> > +
> > /* Helper for gr_set_halt and gr_set_wedge */
> > static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
> > {
> > diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
> > index 70134239179e..14a18d5b5cf8 100644
> > --- a/drivers/usb/gadget/udc/gr_udc.h
> > +++ b/drivers/usb/gadget/udc/gr_udc.h
> > @@ -159,7 +159,6 @@ struct gr_ep {
> > };
> >
> > struct gr_request {
> > -     struct usb_request req;
> >       struct list_head queue;
> >
> >       /* Chain of dma descriptors */
> > @@ -171,6 +170,7 @@ struct gr_request {
> >       u16 oddlen; /* Size of odd length tail if buffer length is "odd" */
> >
> >       u8 setup; /* Setup packet */
> > +     struct usb_request req;
> > };
> >
> > enum gr_ep0state {
> > --
> > 2.25.1
>
> So, to follow the proposed solution in this thread, the following change
> is the "correct" one to make, right?
>
>
> diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
> index 4b35739d3695..5d65d8ad5281 100644
> --- a/drivers/usb/gadget/udc/gr_udc.c
> +++ b/drivers/usb/gadget/udc/gr_udc.c
> @@ -1690,7 +1690,8 @@ static int gr_queue_ext(struct usb_ep *_ep, struct usb_request *_req,
>  /* Dequeue JUST ONE request */
>  static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>  {
> -       struct gr_request *req;
> +       struct gr_request *req = NULL;
> +       struct gr_request *tmp;
>         struct gr_ep *ep;
>         struct gr_udc *dev;
>         int ret = 0;
> @@ -1710,11 +1711,13 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>         spin_lock_irqsave(&dev->lock, flags);
>
>         /* Make sure it's actually queued on this endpoint */
> -       list_for_each_entry(req, &ep->queue, queue) {
> -               if (&req->req == _req)
> +       list_for_each_entry(tmp, &ep->queue, queue) {
> +               if (&tmp->req == _req) {
> +                       req = tmp;
>                         break;
> +               }
>         }
> -       if (&req->req != _req) {
> +       if (!req) {
>                 ret = -EINVAL;
>                 goto out;
>         }

Powered by blists - more mailing lists