[PATCH] usb: gadget: aspeed_udc: fix u16 truncation of descriptor lengths

[PATCH] usb: gadget: aspeed_udc: fix u16 truncation of descriptor lengths

[Liu Chao]

ast_udc_epn_kick_desc() computes tx_len as u32 bounded by
AST_EP_DMA_DESC_MAX_LEN * AST_UDC_DESCS_COUNT (7680 * 256, roughly
1.88 MB) and hands it to ast_dma_descriptor_setup(), whose tx_len
parameter is u16.  The length is truncated to the low 16 bits before
the descriptor list is built, so a descriptor-mode IN request larger
than 64 KB is described -- and therefore transferred -- as only
tx_len % 64 KB bytes.

The caller still accounts the full, untruncated length:

	req->actual_dma_length += tx_len;

so req->actual_dma_length never equals req->req.actual and the "more
requests" check in ast_udc_epn_handle_desc() never kicks the next
stage.  The request never completes and the endpoint queue stops
draining.

The same handler accumulates the per-descriptor transfer lengths of a
stage into a u16 total_len.  Each descriptor carries up to 8191 bytes
(EP_DESC1_IN_LEN mask 0x1fff) and up to AST_UDC_DESCS_COUNT (256)
descriptors are walked, so once the truncation above is fixed the sum
can exceed the u16 range: a full stage covers 7680 * 256 bytes, which
wraps modulo 64 KB to a value smaller than ep->ep.maxpacket.  The
short-packet check then believes the stage was short, completes the
request before the whole transfer has finished, and adds a length to
req->req.actual that does not match what was really moved.

Widen both the tx_len parameter and the total_len accumulator to u32.

Fixes: 055276c13205 ("usb: gadget: add Aspeed ast2600 udc driver")
Cc: stable@vger.kernel.org
Reviewed-by: Guanglei Zhu <zhugl3@xiaopeng.com>
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
 drivers/usb/gadget/udc/aspeed_udc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
index 54f81e668009..f8c58158bd51 100644
--- a/drivers/usb/gadget/udc/aspeed_udc.c
+++ b/drivers/usb/gadget/udc/aspeed_udc.c
@@ -472,7 +472,7 @@ static void ast_udc_ep_free_request(struct usb_ep *_ep,
 }
 
 static int ast_dma_descriptor_setup(struct ast_udc_ep *ep, u32 dma_buf,
-				    u16 tx_len, struct ast_udc_request *req)
+				    u32 tx_len, struct ast_udc_request *req)
 {
 	struct ast_udc_dev *udc = ep->udc;
 	struct device *dev = &udc->pdev->dev;
@@ -899,7 +899,7 @@ static void ast_udc_epn_handle_desc(struct ast_udc_dev *udc, u16 ep_num)
 	struct ast_udc_request *req;
 	u32 proc_sts, wr_ptr, rd_ptr;
 	u32 len_in_desc, ctrl;
-	u16 total_len = 0;
+	u32 total_len = 0;
 	int i;
 
 	if (list_empty(&ep->queue)) {
-- 
2.50.1

Re: [PATCH] usb: gadget: aspeed_udc: fix u16 truncation of descriptor lengths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: gadget: aspeed_udc: DMA ring wrap-around in ast_dma_descriptor_setup()
--

commit 779c6cb173f5ebef2c19640cedeba9f7a9ebcb90
Author: Liu Chao <liuc63@xiaopeng.com>

usb: gadget: aspeed_udc: fix u16 truncation of descriptor lengths

This commit widens the tx_len parameter of ast_dma_descriptor_setup() and
the total_len accumulator in ast_udc_epn_handle_desc() from u16 to u32 so
that descriptor-mode transfers larger than 64 KB are not truncated.

> diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> index 81fa495bbacd1..b8bb81cbb6a6f 100644
> --- a/drivers/usb/gadget/udc/aspeed_udc.c
> +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> @@ -472,7 +472,7 @@ static void ast_udc_ep_free_request(struct usb_ep *_ep,
>  }
>  
>  static int ast_dma_descriptor_setup(struct ast_udc_ep *ep, u32 dma_buf,
> -				    u16 tx_len, struct ast_udc_request *req)
> +				    u32 tx_len, struct ast_udc_request *req)

[Severity: High]
With tx_len widened to u32, can a transfer larger than
AST_EP_DMA_DESC_MAX_LEN * (AST_UDC_DESCS_COUNT - 1) (7680 * 255 bytes) wrap
ep->descs_wptr all the way back to the hardware read pointer and stall the
endpoint queue?

In ast_udc_epn_kick_desc(), tx_len is clamped to descs_max_size, which allows
up to all 256 descriptors in a single stage:

drivers/usb/gadget/udc/aspeed_udc.c:ast_udc_epn_kick_desc() {
	descs_max_size = AST_EP_DMA_DESC_MAX_LEN * AST_UDC_DESCS_COUNT;

	last = req->req.length - req->req.actual;
	tx_len = last > descs_max_size ? descs_max_size : last;
	...
	if (!ast_dma_descriptor_setup(ep, dma_buf, tx_len, req))
		req->actual_dma_length += tx_len;

	/* make sure CPU done everything before triggering DMA */
	mb();

	ast_ep_write(ep, ep->descs_wptr, AST_UDC_EP_DMA_STS);
}

When tx_len > 7680 * 255 (including the full 7680 * 256 byte stage described
in the commit message), the loop in ast_dma_descriptor_setup() runs all
AST_UDC_DESCS_COUNT (256) iterations:

drivers/usb/gadget/udc/aspeed_udc.c:ast_dma_descriptor_setup() {
	while (chunk >= 0 && !last && count < AST_UDC_DESCS_COUNT) {
		...
		if (count == 0)
			req->saved_dma_wptr = ep->descs_wptr;

		ep->descs_wptr++;
		count++;

		if (ep->descs_wptr >= AST_UDC_DESCS_COUNT)
			ep->descs_wptr = 0;

		offset = ep->chunk_max * count;
	}
}

Because the descriptor ring is idle when ast_udc_epn_kick_desc() is called,
ep->descs_wptr starts equal to the hardware RPTR. Advancing ep->descs_wptr
256 times and wrapping at AST_UDC_DESCS_COUNT returns ep->descs_wptr to that
same initial value (req->saved_dma_wptr == RPTR), which is then written to
AST_UDC_EP_DMA_STS.

In the Aspeed endpoint DMA hardware, WPTR == RPTR in AST_UDC_EP_DMA_STS
indicates an empty descriptor list, as checked in ast_udc_epn_handle_desc():

	ctrl = ast_ep_read(ep, AST_UDC_EP_DMA_STS);
	rd_ptr = EP_DMA_GET_RPTR(ctrl);
	wr_ptr = EP_DMA_GET_WPTR(ctrl);

	if (rd_ptr != wr_ptr) {
		dev_warn(dev, "desc list is not empty ! %s:%d, %s:%d\n",
		"rptr", rd_ptr, "wptr", wr_ptr);
		return;
	}

and documented in the sibling Aspeed vHub driver:

drivers/usb/gadget/udc/aspeed-vhub/epn.c:ast_vhub_count_free_descs() {
	/*
	 * d_next == d_last means descriptor list empty to HW,
	 * thus we can only have AST_VHUB_DESCS_COUNT-1 descriptors
	 * in the list
	 */
	return (ep->epn.d_last + AST_VHUB_DESCS_COUNT - ep->epn.d_next - 1) &
		(AST_VHUB_DESCS_COUNT - 1);
}

Should ast_udc_epn_kick_desc() and ast_dma_descriptor_setup() cap each stage
to AST_UDC_DESCS_COUNT - 1 descriptors so that ep->descs_wptr never wraps
onto the hardware RPTR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027024029.1310175-1-liuc63@xiaopeng.com?part=1