usb: gadget: aspeed_udc: list iterator used after loop in ast_udc_ep_dequeue

usb: gadget: aspeed_udc: list iterator used after loop in ast_udc_ep_dequeue

[Xie Maoyi]

[-- Attachment #1: Type: text/plain, Size: 3295 bytes --]

Hi all,

I have been running a small static check for list_for_each_entry
past-the-end patterns, similar to Jakob Koschel's 2022 cleanup
(commit 2966a9918df and related). The check flagged
ast_udc_ep_dequeue() in drivers/usb/gadget/udc/aspeed_udc.c, and I
would like to ask whether you consider this a real defect before I
send anything formal. The same code is present in v7.0 and in
v7.1-rc1 (the two files are byte-identical).

The code in question is around line 691:

    struct ast_udc_request *req;
    ...
    list_for_each_entry(req, &ep->queue, queue) {
        if (&req->req == _req) {
            list_del_init(&req->queue);
            ast_udc_done(ep, req, -ESHUTDOWN);
            _req->status = -ECONNRESET;
            break;
        }
    }
    if (&req->req != _req)
        rc = -EINVAL;

If nothing matches, the loop exits past-the-end and req becomes the
synthetic container_of(&ep->queue, struct ast_udc_request, queue).
Reading &req->req after the loop is undefined per C11. The post-loop
check works in practice only because real _req values do not collide
with that synthetic address.

What made me suspect this was not intentional is that 14 other UDC
drivers in the same directory (at91_udc, atmel_usba_udc, dummy_hcd,
fsl_qe_udc, fsl_udc_core, goku_udc, gr_udc, lpc32xx_udc, max3420_udc,
net2280, omap_udc, pxa25x_udc, pxa27x_udc, udc-xilinx) use a
different pattern, with a separate iter cursor and a result variable.
For example dummy_hcd.c:

    struct dummy_request *req = NULL, *iter;
    list_for_each_entry(iter, &ep->queue, queue) {
        if (&iter->req != _req) continue;
        ...
        req = iter;
        retval = 0;
        break;
    }
    if (retval == 0) { ... }

aspeed_udc seems to be the only outlier in drivers/usb/gadget/udc/,
which is what made me think this was probably an oversight rather
than a deliberate idiom.

I also tried to confirm whether it observably misbehaves. If _req
happens to coincide with the synthetic past-the-end address, the
function returns 0 (success) on an empty queue without removing
anything. I attached a small userspace reproducer (poc_aspeed_udc.c
and its output log) that arranges this collision. In normal use _req
comes from the kernel slab and the collision is unlikely to happen
naturally, so I am not sure whether this rises to the level of a
real bug or just a code-quality issue.

Two questions:

  1. Do you consider the past-the-end use here a defect worth fixing,
     or is it an accepted idiom in this driver that I am misreading?

  2. If it is worth fixing, I already have a small patch that brings
     the function in line with the 14 sibling drivers. Would you like
     me to send it, or would you rather address it locally?

Thanks for taking a look, and apologies if I am off base on any of
this.

Best,
Maoyi Xie
--
Nanyang Technological University
https://maoyixie.com/
________________________________

CONFIDENTIALITY: This email is intended solely for the person(s) named and may be confidential and/or privileged. If you are not the intended recipient, please delete it, notify us and do not copy, use, or disclose its contents.
Towards a sustainable earth: Print only when necessary. Thank you.

[-- Attachment #2: poc_aspeed_udc.log --]
[-- Type: application/octet-stream, Size: 402 bytes --]

$ ./poc_aspeed_udc
[setup] ep.queue=0x7ffefe0b1cd0 (head)
[setup] past_end=0x7ffefe0b1cc0
[setup] fake_req=0x7ffefe0b1cc0
[probe] existing rc=0
[result] returned 0 (success) on empty queue without removing anything

$ ./poc_aspeed_udc patched
[setup] ep.queue=0x7ffee648eee0 (head)
[setup] past_end=0x7ffee648eed0
[setup] fake_req=0x7ffee648eed0
[probe] patched rc=-22
[result] returned -22 (rejected)

[-- Attachment #3: poc_aspeed_udc.c --]
[-- Type: text/plain, Size: 4521 bytes --]

/*
 * Userspace reproducer for the past-the-end iterator behavior in
 * ast_udc_ep_dequeue() (drivers/usb/gadget/udc/aspeed_udc.c).
 *
 * Aspeed UDC is BMC/ARM hardware. Rather than bringing up a full SoC
 * emulation, this program extracts the dequeue function's logic into
 * userspace using mock structs whose layout (req at offset 0, queue
 * immediately after) matches the kernel definition. It then runs both
 * the existing code path and the proposed fix on the same crafted input.
 *
 * Build: cc -O0 -g poc_aspeed_udc.c -o poc_aspeed_udc
 * Run:   ./poc_aspeed_udc           (existing code, returns 42)
 *        ./poc_aspeed_udc patched   (proposed fix, returns 0)
 */
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stddef.h>

/* Minimal mock of the kernel list_head and container_of. */
struct list_head { struct list_head *next, *prev; };

#define container_of(ptr, type, member) \
    ((type *)((char *)(ptr) - offsetof(type, member)))

#define list_first_entry(ptr, type, member) \
    container_of((ptr)->next, type, member)
#define list_next_entry(pos, member) \
    container_of((pos)->member.next, typeof(*(pos)), member)
#define list_entry_is_head(pos, head, member) \
    (&(pos)->member == (head))

#define list_for_each_entry(pos, head, member)               \
    for (pos = list_first_entry(head, typeof(*pos), member); \
         !list_entry_is_head(pos, head, member);             \
         pos = list_next_entry(pos, member))

static void list_init(struct list_head *h) { h->next = h->prev = h; }

/* Mock structs. Only field order matters: req at offset 0, queue
 * immediately after. */
struct usb_request {
    void *buf;
    unsigned length;
    int status;
};

struct ast_udc_request {
    struct usb_request req;
    struct list_head queue;
    int pad;
};

struct ast_udc_ep {
    struct list_head queue;
};

/* Existing code path from aspeed_udc.c around line 691. Locks and
 * the ast_udc_done() callback are elided since the past-the-end
 * behavior is independent of them. */
static int ast_udc_ep_dequeue_existing(struct ast_udc_ep *ep,
                                        struct usb_request *_req)
{
    struct ast_udc_request *req;
    int rc = 0;

    list_for_each_entry(req, &ep->queue, queue) {
        if (&req->req == _req) {
            /* list_del_init + ast_udc_done + set status here */
            break;
        }
    }

    /* When the loop finds no match, req is past-the-end. Reading
     * &req->req is undefined per C11; the resulting check is a
     * property of heap layout rather than the queue contents. */
    if (&req->req != _req)
        rc = -22;     /* -EINVAL */

    return rc;
}

/* Proposed fix using the separate iter cursor pattern shared by the
 * other UDC drivers in the same directory (e.g. dummy_hcd.c). */
static int ast_udc_ep_dequeue_patched(struct ast_udc_ep *ep,
                                       struct usb_request *_req)
{
    struct ast_udc_request *req = NULL, *iter;

    list_for_each_entry(iter, &ep->queue, queue) {
        if (&iter->req != _req)
            continue;
        req = iter;
        break;
    }

    if (!req)
        return -22;     /* -EINVAL */

    /* list_del_init + ast_udc_done + set status here */
    return 0;
}

int main(int argc, char **argv)
{
    int use_patched = (argc > 1 && !strcmp(argv[1], "patched"));

    struct ast_udc_ep ep;
    list_init(&ep.queue);

    /* An empty queue forces the existing code's iterator past the end.
     * past_end is the synthetic ast_udc_request pointer the loop will
     * leave behind. Because req is the first member, &past_end->req
     * has the same numeric value as past_end itself. */
    struct ast_udc_request *past_end =
        container_of(&ep.queue, struct ast_udc_request, queue);
    struct usb_request *fake_req = &past_end->req;

    printf("[setup] ep.queue=%p (head)\n", (void *)&ep.queue);
    printf("[setup] past_end=%p\n", (void *)past_end);
    printf("[setup] fake_req=%p\n", (void *)fake_req);

    int rc;
    if (use_patched) {
        rc = ast_udc_ep_dequeue_patched(&ep, fake_req);
        printf("[probe] patched rc=%d\n", rc);
    } else {
        rc = ast_udc_ep_dequeue_existing(&ep, fake_req);
        printf("[probe] existing rc=%d\n", rc);
    }

    if (rc == 0) {
        printf("[result] returned 0 (success) on empty queue without "
               "removing anything\n");
        return 42;
    }
    printf("[result] returned %d (rejected)\n", rc);
    return 0;
}

Re: usb: gadget: aspeed_udc: list iterator used after loop in ast_udc_ep_dequeue

[gregkh]

On Sun, May 17, 2026 at 02:03:47PM +0000, Xie Maoyi wrote:
> CONFIDENTIALITY: This email is intended solely for the person(s) named and may be confidential and/or privileged. If you are not the intended recipient, please delete it, notify us and do not copy, use, or disclose its contents.

Now deleted.

usb: gadget: aspeed_udc: list iterator used after loop in ast_udc_ep_dequeue

[Maoyi Xie]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

Hi all,

(Resending from a personal address — my previous attempt from
my NTU corporate account carried an auto-appended confidentiality
disclaimer that you've declined to accept. The content below is
unchanged.)

I have been running a small static check for list_for_each_entry
past-the-end patterns, similar to Jakob Koschel's 2022 cleanup
(commit 2966a9918df and related). The check flagged
ast_udc_ep_dequeue() in drivers/usb/gadget/udc/aspeed_udc.c, and I
would like to ask whether you consider this a real defect before I
send anything formal. The same code is present in v7.0 and in
v7.1-rc1 (the two files are byte-identical).

The code in question is around line 691:

    struct ast_udc_request *req;
    ...
    list_for_each_entry(req, &ep->queue, queue) {
        if (&req->req == _req) {
            list_del_init(&req->queue);
            ast_udc_done(ep, req, -ESHUTDOWN);
            _req->status = -ECONNRESET;
            break;
        }
    }
    if (&req->req != _req)
        rc = -EINVAL;

If nothing matches, the loop exits past-the-end and req becomes the
synthetic container_of(&ep->queue, struct ast_udc_request, queue).
Reading &req->req after the loop is undefined per C11. The post-loop
check works in practice only because real _req values do not collide
with that synthetic address.

What made me suspect this was not intentional is that 14 other UDC
drivers in the same directory (at91_udc, atmel_usba_udc, dummy_hcd,
fsl_qe_udc, fsl_udc_core, goku_udc, gr_udc, lpc32xx_udc, max3420_udc,
net2280, omap_udc, pxa25x_udc, pxa27x_udc, udc-xilinx) use a
different pattern, with a separate iter cursor and a result variable.
For example dummy_hcd.c:

    struct dummy_request *req = NULL, *iter;
    list_for_each_entry(iter, &ep->queue, queue) {
        if (&iter->req != _req) continue;
        ...
        req = iter;
        retval = 0;
        break;
    }
    if (retval == 0) { ... }

aspeed_udc seems to be the only outlier in drivers/usb/gadget/udc/,
which is what made me think this was probably an oversight rather
than a deliberate idiom.

I also tried to confirm whether it observably misbehaves. If _req
happens to coincide with the synthetic past-the-end address, the
function returns 0 (success) on an empty queue without removing
anything. I attached a small userspace reproducer (poc_aspeed_udc.c
and its output log) that arranges this collision. In normal use _req
comes from the kernel slab and the collision is unlikely to happen
naturally, so I am not sure whether this rises to the level of a
real bug or just a code-quality issue.

Two questions:

  1. Do you consider the past-the-end use here a defect worth fixing,
     or is it an accepted idiom in this driver that I am misreading?

  2. If it is worth fixing, I already have a small patch that brings
     the function in line with the 14 sibling drivers. Would you like
     me to send it, or would you rather address it locally?

Thanks for taking a look, and apologies if I am off base on any of
this.

Best,
Maoyi Xie
--
Nanyang Technological University
https://maoyixie.com/

[-- Attachment #2: poc_aspeed_udc.c --]
[-- Type: application/octet-stream, Size: 4521 bytes --]

/*
 * Userspace reproducer for the past-the-end iterator behavior in
 * ast_udc_ep_dequeue() (drivers/usb/gadget/udc/aspeed_udc.c).
 *
 * Aspeed UDC is BMC/ARM hardware. Rather than bringing up a full SoC
 * emulation, this program extracts the dequeue function's logic into
 * userspace using mock structs whose layout (req at offset 0, queue
 * immediately after) matches the kernel definition. It then runs both
 * the existing code path and the proposed fix on the same crafted input.
 *
 * Build: cc -O0 -g poc_aspeed_udc.c -o poc_aspeed_udc
 * Run:   ./poc_aspeed_udc           (existing code, returns 42)
 *        ./poc_aspeed_udc patched   (proposed fix, returns 0)
 */
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stddef.h>

/* Minimal mock of the kernel list_head and container_of. */
struct list_head { struct list_head *next, *prev; };

#define container_of(ptr, type, member) \
    ((type *)((char *)(ptr) - offsetof(type, member)))

#define list_first_entry(ptr, type, member) \
    container_of((ptr)->next, type, member)
#define list_next_entry(pos, member) \
    container_of((pos)->member.next, typeof(*(pos)), member)
#define list_entry_is_head(pos, head, member) \
    (&(pos)->member == (head))

#define list_for_each_entry(pos, head, member)               \
    for (pos = list_first_entry(head, typeof(*pos), member); \
         !list_entry_is_head(pos, head, member);             \
         pos = list_next_entry(pos, member))

static void list_init(struct list_head *h) { h->next = h->prev = h; }

/* Mock structs. Only field order matters: req at offset 0, queue
 * immediately after. */
struct usb_request {
    void *buf;
    unsigned length;
    int status;
};

struct ast_udc_request {
    struct usb_request req;
    struct list_head queue;
    int pad;
};

struct ast_udc_ep {
    struct list_head queue;
};

/* Existing code path from aspeed_udc.c around line 691. Locks and
 * the ast_udc_done() callback are elided since the past-the-end
 * behavior is independent of them. */
static int ast_udc_ep_dequeue_existing(struct ast_udc_ep *ep,
                                        struct usb_request *_req)
{
    struct ast_udc_request *req;
    int rc = 0;

    list_for_each_entry(req, &ep->queue, queue) {
        if (&req->req == _req) {
            /* list_del_init + ast_udc_done + set status here */
            break;
        }
    }

    /* When the loop finds no match, req is past-the-end. Reading
     * &req->req is undefined per C11; the resulting check is a
     * property of heap layout rather than the queue contents. */
    if (&req->req != _req)
        rc = -22;     /* -EINVAL */

    return rc;
}

/* Proposed fix using the separate iter cursor pattern shared by the
 * other UDC drivers in the same directory (e.g. dummy_hcd.c). */
static int ast_udc_ep_dequeue_patched(struct ast_udc_ep *ep,
                                       struct usb_request *_req)
{
    struct ast_udc_request *req = NULL, *iter;

    list_for_each_entry(iter, &ep->queue, queue) {
        if (&iter->req != _req)
            continue;
        req = iter;
        break;
    }

    if (!req)
        return -22;     /* -EINVAL */

    /* list_del_init + ast_udc_done + set status here */
    return 0;
}

int main(int argc, char **argv)
{
    int use_patched = (argc > 1 && !strcmp(argv[1], "patched"));

    struct ast_udc_ep ep;
    list_init(&ep.queue);

    /* An empty queue forces the existing code's iterator past the end.
     * past_end is the synthetic ast_udc_request pointer the loop will
     * leave behind. Because req is the first member, &past_end->req
     * has the same numeric value as past_end itself. */
    struct ast_udc_request *past_end =
        container_of(&ep.queue, struct ast_udc_request, queue);
    struct usb_request *fake_req = &past_end->req;

    printf("[setup] ep.queue=%p (head)\n", (void *)&ep.queue);
    printf("[setup] past_end=%p\n", (void *)past_end);
    printf("[setup] fake_req=%p\n", (void *)fake_req);

    int rc;
    if (use_patched) {
        rc = ast_udc_ep_dequeue_patched(&ep, fake_req);
        printf("[probe] patched rc=%d\n", rc);
    } else {
        rc = ast_udc_ep_dequeue_existing(&ep, fake_req);
        printf("[probe] existing rc=%d\n", rc);
    }

    if (rc == 0) {
        printf("[result] returned 0 (success) on empty queue without "
               "removing anything\n");
        return 42;
    }
    printf("[result] returned %d (rejected)\n", rc);
    return 0;
}

[-- Attachment #3: poc_aspeed_udc.log --]
[-- Type: application/octet-stream, Size: 402 bytes --]

$ ./poc_aspeed_udc
[setup] ep.queue=0x7ffefe0b1cd0 (head)
[setup] past_end=0x7ffefe0b1cc0
[setup] fake_req=0x7ffefe0b1cc0
[probe] existing rc=0
[result] returned 0 (success) on empty queue without removing anything

$ ./poc_aspeed_udc patched
[setup] ep.queue=0x7ffee648eee0 (head)
[setup] past_end=0x7ffee648eed0
[setup] fake_req=0x7ffee648eed0
[probe] patched rc=-22
[result] returned -22 (rejected)

Re: usb: gadget: aspeed_udc: list iterator used after loop in ast_udc_ep_dequeue

[Andrew Jeffery]

Hi,

On Sun, 2026-05-17 at 23:46 +0800, Maoyi Xie wrote:
> Hi all,
> 
> (Resending from a personal address — my previous attempt from
> my NTU corporate account carried an auto-appended confidentiality
> disclaimer that you've declined to accept. The content below is
> unchanged.)
> 
> I have been running a small static check for list_for_each_entry
> past-the-end patterns, similar to Jakob Koschel's 2022 cleanup
> (commit 2966a9918df and related). The check flagged
> ast_udc_ep_dequeue() in drivers/usb/gadget/udc/aspeed_udc.c, and I
> would like to ask whether you consider this a real defect before I
> send anything formal. The same code is present in v7.0 and in
> v7.1-rc1 (the two files are byte-identical).
> 
> The code in question is around line 691:
> 
>     struct ast_udc_request *req;
>     ...
>     list_for_each_entry(req, &ep->queue, queue) {
>         if (&req->req == _req) {
>             list_del_init(&req->queue);
>             ast_udc_done(ep, req, -ESHUTDOWN);
>             _req->status = -ECONNRESET;
>             break;
>         }
>     }
>     if (&req->req != _req)
>         rc = -EINVAL;
> 
> If nothing matches, the loop exits past-the-end and req becomes the
> synthetic container_of(&ep->queue, struct ast_udc_request, queue).
> Reading &req->req after the loop is undefined per C11. The post-loop
> check works in practice only because real _req values do not collide
> with that synthetic address.
> 
> What made me suspect this was not intentional is that 14 other UDC
> drivers in the same directory (at91_udc, atmel_usba_udc, dummy_hcd,
> fsl_qe_udc, fsl_udc_core, goku_udc, gr_udc, lpc32xx_udc, max3420_udc,
> net2280, omap_udc, pxa25x_udc, pxa27x_udc, udc-xilinx) use a
> different pattern, with a separate iter cursor and a result variable.
> For example dummy_hcd.c:
> 
>     struct dummy_request *req = NULL, *iter;
>     list_for_each_entry(iter, &ep->queue, queue) {
>         if (&iter->req != _req) continue;
>         ...
>         req = iter;
>         retval = 0;
>         break;
>     }
>     if (retval == 0) { ... }
> 
> aspeed_udc seems to be the only outlier in drivers/usb/gadget/udc/,
> which is what made me think this was probably an oversight rather
> than a deliberate idiom.
> 
> I also tried to confirm whether it observably misbehaves. If _req
> happens to coincide with the synthetic past-the-end address, the
> function returns 0 (success) on an empty queue without removing
> anything. I attached a small userspace reproducer (poc_aspeed_udc.c
> and its output log) that arranges this collision. In normal use _req
> comes from the kernel slab and the collision is unlikely to happen
> naturally, so I am not sure whether this rises to the level of a
> real bug or just a code-quality issue.
> 
> Two questions:
> 
>   1. Do you consider the past-the-end use here a defect worth fixing,
>      or is it an accepted idiom in this driver that I am misreading?

I don't know that it's an accepted idiom - there are only two
invocations in the driver and the other doesn't suffer the problem
you've highlighted.

list_first_entry() does note that the caller be sure that the list
isn't empty. As you note this isn't tested, so it's now a pre-condition
of ast_udc_ep_dequeue() that it's not. A bunch of gadgets test if they
have requests in-flight before invoking dequeue, so that may not be
unreasonable. However, given ast_udc_nuke(), and the implementation
admitting that the provided req might be invalid by the existence of
the test, I'm not convinced that the invariant is upheld.

Note that the (other, separate) Aspeed vhub driver avoids your concern
in its dequeue implementations, so I'd rather it's avoided in the udc
implementation as well:

 * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/udc/aspeed-vhub/ep0.c?h=v7.1-rc4#n438
 * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/udc/aspeed-vhub/epn.c?h=v7.1-rc4#n472

> 
>   2. If it is worth fixing, I already have a small patch that brings
>      the function in line with the 14 sibling drivers. Would you like
>      me to send it, or would you rather address it locally?

IMO send the patch. It's almost always better to be reviewing a
concrete change proposal.

Andrew