[PATCH] usb: gadget: aspeed_udc: check endpoint DMA allocation

Ruoyu Wang <[email protected]>
Newsgroups org.ozlabs.lists.linux-aspeed,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-usb
Message-ID <[email protected]>
ast_udc_probe() allocates a coherent DMA buffer used as the backing store
for endpoint buffers. ast_udc_init_ep() derives per-endpoint buffer
pointers from udc->ep0_buf, so a failed allocation is dereferenced during
probe.

Check the allocation before endpoint setup. The existing probe error path
called ast_udc_remove(), which unregisters the gadget unconditionally and
is not safe before usb_add_gadget_udc() succeeds. Add a local cleanup
helper for probe failures so pre-registration failures only unwind the
resources that were actually initialized.

This was found by a local static analysis checker for unchecked allocator
returns while scanning Linux 6.16. The change was checked by applying it
to current mainline and by running checkpatch. I do not have access to
Aspeed UDC hardware, so no runtime testing was performed.

Fixes: 055276c13205 ("usb: gadget: add Aspeed ast2600 udc driver")
Signed-off-by: Ruoyu Wang <[email protected]>
---
Note: a 2022 patch attempted to add only a NULL check for this
allocation:
https://lore.kernel.org/all/[email protected]/

This version also fixes the probe unwind path so the clock is disabled
on allocation failure and usb_del_gadget_udc() is not called before the
gadget has been registered.

diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
index 7fc6696b7..809a7d5b7 100644
--- a/drivers/usb/gadget/udc/aspeed_udc.c
+++ b/drivers/usb/gadget/udc/aspeed_udc.c
@@ -1434,11 +1434,34 @@ static void ast_udc_init_hw(struct ast_udc_dev *udc)
 	ast_udc_write(udc, 0, AST_UDC_EP0_CTRL);
 }
 
+static void ast_udc_cleanup(struct platform_device *pdev)
+{
+	struct ast_udc_dev *udc = platform_get_drvdata(pdev);
+	unsigned long flags;
+	u32 ctrl;
+
+	spin_lock_irqsave(&udc->lock, flags);
+
+	/* Disable upstream port connection */
+	ctrl = ast_udc_read(udc, AST_UDC_FUNC_CTRL) & ~USB_UPSTREAM_EN;
+	ast_udc_write(udc, ctrl, AST_UDC_FUNC_CTRL);
+
+	clk_disable_unprepare(udc->clk);
+
+	spin_unlock_irqrestore(&udc->lock, flags);
+
+	if (udc->ep0_buf)
+		dma_free_coherent(&pdev->dev,
+				  AST_UDC_EP_DMA_SIZE * AST_UDC_NUM_ENDPOINTS,
+				  udc->ep0_buf,
+				  udc->ep0_buf_dma);
+
+	udc->ep0_buf = NULL;
+}
+
 static void ast_udc_remove(struct platform_device *pdev)
 {
 	struct ast_udc_dev *udc = platform_get_drvdata(pdev);
-	unsigned long flags;
-	u32 ctrl;
 
 	usb_del_gadget_udc(&udc->gadget);
 	if (udc->driver) {
@@ -1453,23 +1476,7 @@ static void ast_udc_remove(struct platform_device *pdev)
 		return;
 	}
 
-	spin_lock_irqsave(&udc->lock, flags);
-
-	/* Disable upstream port connection */
-	ctrl = ast_udc_read(udc, AST_UDC_FUNC_CTRL) & ~USB_UPSTREAM_EN;
-	ast_udc_write(udc, ctrl, AST_UDC_FUNC_CTRL);
-
-	clk_disable_unprepare(udc->clk);
-
-	spin_unlock_irqrestore(&udc->lock, flags);
-
-	if (udc->ep0_buf)
-		dma_free_coherent(&pdev->dev,
-				  AST_UDC_EP_DMA_SIZE * AST_UDC_NUM_ENDPOINTS,
-				  udc->ep0_buf,
-				  udc->ep0_buf_dma);
-
-	udc->ep0_buf = NULL;
+	ast_udc_cleanup(pdev);
 }
 
 static int ast_udc_probe(struct platform_device *pdev)
@@ -1523,6 +1530,10 @@ static int ast_udc_probe(struct platform_device *pdev)
 					  AST_UDC_EP_DMA_SIZE *
 					  AST_UDC_NUM_ENDPOINTS,
 					  &udc->ep0_buf_dma, GFP_KERNEL);
+	if (!udc->ep0_buf) {
+		rc = -ENOMEM;
+		goto err_disable_clk;
+	}
 
 	udc->gadget.speed = USB_SPEED_UNKNOWN;
 	udc->gadget.max_speed = USB_SPEED_HIGH;
@@ -1553,20 +1564,20 @@ static int ast_udc_probe(struct platform_device *pdev)
 	udc->irq = platform_get_irq(pdev, 0);
 	if (udc->irq < 0) {
 		rc = udc->irq;
-		goto err;
+		goto err_cleanup;
 	}
 
 	rc = devm_request_irq(&pdev->dev, udc->irq, ast_udc_isr, 0,
 			      KBUILD_MODNAME, udc);
 	if (rc) {
 		dev_err(&pdev->dev, "Failed to request interrupt\n");
-		goto err;
+		goto err_cleanup;
 	}
 
 	rc = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
 	if (rc) {
 		dev_err(&pdev->dev, "Failed to add gadget udc\n");
-		goto err;
+		goto err_cleanup;
 	}
 
 	dev_info(&pdev->dev, "Initialized udc in USB%s mode\n",
@@ -1574,9 +1585,14 @@ static int ast_udc_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_disable_clk:
+	clk_disable_unprepare(udc->clk);
+	goto err;
+err_cleanup:
+	ast_udc_cleanup(pdev);
+	goto err;
 err:
 	dev_err(&pdev->dev, "Failed to udc probe, rc:0x%x\n", rc);
-	ast_udc_remove(pdev);
 
 	return rc;
 }

-- 
2.51.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.