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