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

[sashiko-bot@kernel.org]

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