[PATCH] usb: gadget: pch_udc: Fix potential ep0 DMA descriptor leak

[PATCH] usb: gadget: pch_udc: Fix potential ep0 DMA descriptor leak

[Guangshuo Li]

Removal destroys data_requests while the ep0 data descriptor is still
allocated, then tries to return that descriptor to stp_requests. The
busy data pool cannot release its DMA backing memory.

Return the descriptor to data_requests before destroying that pool.

This issue was found by our static analysis tool under development.

Fixes: f646cf94520e ("USB device driver of Topcliff PCH")
Signed-off-by: Guangshuo Li <liguangshuo@mail.sdu.edu.cn>
---
 drivers/usb/gadget/udc/pch_udc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index 99b3ce28210f..5996f1d16aed 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -2989,6 +2989,10 @@ static void pch_udc_remove(struct pci_dev *pdev)
 		dev_err(&pdev->dev,
 			"%s: gadget driver still bound!!!\n", __func__);
 	/* dma pool cleanup */
+	if (dev->ep[UDC_EP0OUT_IDX].td_data)
+		dma_pool_free(dev->data_requests,
+			      dev->ep[UDC_EP0OUT_IDX].td_data,
+			      dev->ep[UDC_EP0OUT_IDX].td_data_phys);
 	dma_pool_destroy(dev->data_requests);
 
 	if (dev->stp_requests) {
@@ -2998,11 +3002,6 @@ static void pch_udc_remove(struct pci_dev *pdev)
 				dev->ep[UDC_EP0OUT_IDX].td_stp,
 				dev->ep[UDC_EP0OUT_IDX].td_stp_phys);
 		}
-		if (dev->ep[UDC_EP0OUT_IDX].td_data) {
-			dma_pool_free(dev->stp_requests,
-				dev->ep[UDC_EP0OUT_IDX].td_data,
-				dev->ep[UDC_EP0OUT_IDX].td_data_phys);
-		}
 		dma_pool_destroy(dev->stp_requests);
 	}
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

Re: [PATCH] usb: gadget: pch_udc: Fix potential ep0 DMA descriptor leak

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] usb: gadget: pch_udc: double-put in pch_udc_probe() [1]
--

commit f7d7f9013f20e3f93fc549afedd53c2c94be66c1
Author: Guangshuo Li <liguangshuo@mail.sdu.edu.cn>

usb: gadget: pch_udc: Fix potential ep0 DMA descriptor leak

This commit frees the ep0 data DMA descriptor back to dev->data_requests
before destroying that pool in pch_udc_remove(), instead of freeing it to
dev->stp_requests.

> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index 99b3ce28210f9..5996f1d16aedc 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -2989,6 +2989,10 @@ static void pch_udc_remove(struct pci_dev *pdev)
>  		dev_err(&pdev->dev,
>  			"%s: gadget driver still bound!!!\n", __func__);
>  	/* dma pool cleanup */
> +	if (dev->ep[UDC_EP0OUT_IDX].td_data)
> +		dma_pool_free(dev->data_requests,
> +			      dev->ep[UDC_EP0OUT_IDX].td_data,
> +			      dev->ep[UDC_EP0OUT_IDX].td_data_phys);
>  	dma_pool_destroy(dev->data_requests);
>  
>  	if (dev->stp_requests) {
> @@ -2998,11 +3002,6 @@ static void pch_udc_remove(struct pci_dev *pdev)
>  				dev->ep[UDC_EP0OUT_IDX].td_stp,
>  				dev->ep[UDC_EP0OUT_IDX].td_stp_phys);
>  		}
> -		if (dev->ep[UDC_EP0OUT_IDX].td_data) {
> -			dma_pool_free(dev->stp_requests,
> -				dev->ep[UDC_EP0OUT_IDX].td_data,
> -				dev->ep[UDC_EP0OUT_IDX].td_data_phys);
> -		}
>  		dma_pool_destroy(dev->stp_requests);
>  	}

[Severity: Critical]
This problem wasn't introduced by this patch, but can pch_udc_remove() call
usb_del_gadget_udc() on an uninitialized or already-cleaned &dev->gadget
[1]?

pch_udc_probe() jumps to its finished label on failure and calls
pch_udc_remove(), which unconditionally invokes usb_del_gadget_udc() at the
top of the function:

drivers/usb/gadget/udc/pch_udc.c:pch_udc_probe() {
	...
	retval = init_dma_pools(dev);
	if (retval)
		goto finished;
	...
	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
	if (retval)
		goto finished;
	return 0;

finished:
	pch_udc_remove(pdev);
	return retval;
}

If probe fails before usb_add_gadget_udc(), usb_del_gadget_udc() calls
put_device() on a zero-initialized kobject.  If usb_add_gadget_udc() itself
fails, it already drops its device references before returning, so calling
usb_del_gadget_udc() in pch_udc_remove() causes a double-put and
use-after-free.

[1] https://sashiko.dev/#/bug/linux-7ca8dd59-dc65-4e63-bbde-b83dd0eb2845

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-e5624093d6-usb-pch-udc@mail.sdu.edu.cn?part=1