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

[Xie Maoyi]

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: poc_aspeed_udc.log
Description: poc_aspeed_udc.log

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