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