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;
}
