Re: [PATCH v2] usb: dwc3: gadget: linearize SG requests that exceed the TRB cache

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: dwc3: UAF and memory leak on ISOC transfer start failure
- [High] usb: dwc3: gadget: DMA mapping leak for requests without TRBs
- [High] usb: dwc3: Double DMA mapping leak in dwc3_prepare_trbs() on TRB exhaustion
--

commit 4e03749f5dcd711bf2541cb720afd9cd91d48fb2
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>

usb: dwc3: gadget: linearize SG requests that exceed the TRB cache

The controller prefetches the TRBs of a transfer into an internal cache
that holds DWC_USB3_CACHE_TRBS_PER_TRANSFER entries. Databook 4.2.3.3
requires that the number of chained TRBs needed to construct a single
packet never exceeds (DWC_USB3_CACHE_TRBS_PER_TRANSFER - 1). This patch
walks the scatter-gather list and linearizes requests when a packet
would exceed the budget.

Link: https://lore.kernel.org/all/20261004031445.1450196-1-faisal.hassan@oss.qualcomm.com/

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f245e66cd13d0..7417d3c32dbf2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
[ ... ]
> @@ -202,10 +218,16 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
>  	if (req->request.status == -EINPROGRESS)
>  		req->request.status = status;
>  
> -	if (req->trb)
> +	/*
> +	 * A linearized request may have been mapped without getting a TRB, so
> +	 * unmap it before its buffer is freed.
> +	 */
> +	if (req->trb || req->sg_trb_cache_bounced)
>  		usb_gadget_unmap_request_by_dev(dwc->sysdev,
>  				&req->request, req->direction);
>  

[Severity: High]
This condition incorrectly relies on req->trb to determine if a non-bounced
request was mapped. Will this bypass unmapping for requests that were
mapped but failed to acquire a TRB?

In dwc3_prepare_trbs(), requests in the pending_list are unconditionally
mapped prior to TRB allocation:

drivers/usb/dwc3/gadget.c:dwc3_prepare_trbs() {
	...
	list_for_each_entry_safe(req, n, &dep->pending_list, list) {
		ret = usb_gadget_map_request_by_dev(dwc->sysdev, &req->request,
						    dep->direction);
		if (ret)
			return ret;
		...
		if (!ret || !dwc3_calc_trbs_left(dep))
			return ret;
}

If dwc3_calc_trbs_left() indicates insufficient TRBs, the function returns
early. The mapped request remains on pending_list with req->trb == NULL.

If the request is dequeued (canceled) in this state, it bypasses the unmapping
logic here, leaking the mapping.

Furthermore, every time a single TRB completes, dwc3_prepare_trbs() is
triggered again. It iterates over the same request on pending_list and
maps it again, overwriting the req->dma pointer and leaking the previous
mapping, because usb_gadget_map_request_by_dev() lacks checks for
preexisting mappings (like req->dma_mapped).

[ ... ]
> @@ -1985,6 +2111,7 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  {
>  	struct dwc3		*dwc = dep->dwc;
> +	int			ret;
>  
>  	if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
>  		dev_dbg(dwc->dev, "%s: can't queue to disabled endpoint\n",
> @@ -2001,6 +2128,10 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  				dep->name, &req->request))
>  		return -EINVAL;
>  
> +	ret = dwc3_gadget_linearize_sg_request(dep, req);
> +	if (ret)
> +		return ret;
> +
>  	pm_runtime_get(dwc->dev);
>  
>  	req->request.actual	= 0;
>  	req->request.status	= -EINPROGRESS;
>  
>  	trace_dwc3_ep_queue(req);
>  
>  	list_add_tail(&req->list, &dep->pending_list);
>  	req->status = DWC3_REQUEST_STATUS_QUEUED;
>  
>  	if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE)
>  		return 0;
>  
>  	/*
>  	 * Start the transfer only after the END_TRANSFER is completed
>  	 * and endpoint STALL is cleared.
>  	 */
>  	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
>  	    (dep->flags & DWC3_EP_WEDGE) ||
>  	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>  	    (dep->flags & DWC3_EP_STALL)) {
>  		dep->flags |= DWC3_EP_DELAY_START;
>  		return 0;
>  	}
>  
>  	/*
>  	 * NOTICE: Isochronous endpoints should NEVER be prestarted. We must
>  	 * wait for a XferNotReady event so we will know what's the current
>  	 * (micro-)frame number.
>  	 *
>  	 * Without this trick, we are very, very likely gonna get Bus Expiry
>  	 * errors which will force us issue EndTransfer command.
>  	 */
>  	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
>  		if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) {
>  			if ((dep->flags & DWC3_EP_PENDING_REQUEST))
>  				return __dwc3_gadget_start_isoc(dep);

[Severity: High]
If __dwc3_gadget_start_isoc() fails here (for example, if
usb_gadget_map_request_by_dev() fails in dwc3_prepare_trbs()), the error code
is returned directly from __dwc3_gadget_ep_queue().

Since the request was already appended to dep->pending_list, the error
propagates back to the gadget driver, which assumes the request was not queued
and frees it. Will this lead to a Use-After-Free list corruption the next time
DWC3 processes pending_list?

Additionally, does this leak the newly allocated sg_trb_cache_buf bounce buffer
and the PM usage count because dwc3_gadget_restore_sg_trb_cache_buf() and
pm_runtime_put() are skipped when returning the error directly?

>  
>  			return 0;
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006172254.1058992-1-faisal.hassan@oss.qualcomm.com?part=1