[PATCH v2] usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

[PATCH v2] usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

[Ma Ke]

cdns3_get_second_fun() looks up the second PCI function with
pci_get_device(), which returns the device with a reference held, and
hands it back to its callers.  Neither cdns3_pci_probe() nor
cdns3_pci_remove() ever drops that reference, so the second function's
pci_dev can never be released.

The cdnsp-pci.c driver handles the same situation correctly.
cdnsp_pci_probe() jumps to a 'put_pci:' label that calls
pci_dev_put(), and cdnsp_pci_remove() calls pci_dev_put() as well.

While at it, also add the missing NULL check in cdns3_pci_remove().
pci_is_enabled() dereferences its argument, so if
cdns3_get_second_fun() returns NULL, the current code may crash.
cdnsp_pci_remove() guards with "!func ||".

cdns3_pci_probe() gained one more return path since this was first
posted: commit e5052f2c5c73 ("usb: cdns3: Fix NULL pointer
dereference in cdns3_pci_probe") added a -EPROBE_DEFER return after
the reference has been taken, so that path needs to drop it as well.

Found by code review.

Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Cc: stable@vger.kernel.org
Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
Changes in v2:
- rebase on current mainline. cdns3_pci_probe() gained a -EPROBE_DEFER
  path in e5052f2c5c73 that returns after the reference has been taken, 
  so it has to drop it too.
---
 drivers/usb/cdns3/cdns3-pci-wrap.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
index fd06a3d8e638..7799349bba77 100644
--- a/drivers/usb/cdns3/cdns3-pci-wrap.c
+++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
@@ -89,7 +89,7 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
 	err = pcim_enable_device(pdev);
 	if (err) {
 		dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
-		return err;
+		goto put_pci;
 	}
 
 	pci_set_master(pdev);
@@ -99,12 +99,15 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
 		if (!wrap) {
 			dev_err(&pdev->dev,
 				"second function not initialized, retrying\n");
-			return -EPROBE_DEFER;
+			err = -EPROBE_DEFER;
+			goto put_pci;
 		}
 	} else {
 		wrap = kzalloc_obj(*wrap);
-		if (!wrap)
-			return -ENOMEM;
+		if (!wrap) {
+			err = -ENOMEM;
+			goto put_pci;
+		}
 	}
 
 	res = wrap->dev_res;
@@ -165,11 +168,13 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
 		if (IS_ERR(wrap->plat_dev)) {
 			err = PTR_ERR(wrap->plat_dev);
 			kfree(wrap);
-			return err;
+			goto put_pci;
 		}
 	}
 
 	pci_set_drvdata(pdev, wrap);
+put_pci:
+	pci_dev_put(func);
 	return err;
 }
 
@@ -184,8 +189,10 @@ static void cdns3_pci_remove(struct pci_dev *pdev)
 	if (wrap->devfn == pdev->devfn)
 		platform_device_unregister(wrap->plat_dev);
 
-	if (!pci_is_enabled(func))
+	if (!func || !pci_is_enabled(func))
 		kfree(wrap);
+
+	pci_dev_put(func);
 }
 
 static const struct pci_device_id cdns3_pci_ids[] = {
-- 
2.43.0

Re: [PATCH v2] usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: cdns3: UAF in cdns3_pci_probe() error path freeing shared drvdata
--

commit 4c0bd2a76b0d6740fa24b374ecb3e7b1b96c27ce
Author: Ma Ke <make_ruc2021@163.com>

usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

This commit addresses a PCI device reference leak in the cdns3 driver by
ensuring pci_dev_put() is called on the device returned by
cdns3_get_second_fun(). It updates the probe and remove functions to
properly release the reference, including in error paths. A missing NULL
check before pci_is_enabled() in the remove function is also added.

> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
> index fd06a3d8e638f..7799349bba771 100644
> --- a/drivers/usb/cdns3/cdns3-pci-wrap.c
> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
[ ... ]
> @@ -165,11 +168,13 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
>  		if (IS_ERR(wrap->plat_dev)) {
>  			err = PTR_ERR(wrap->plat_dev);
>  			kfree(wrap);
> -			return err;
> +			goto put_pci;
>  		}
>  	}

[Severity: High]
Can this error path free a shared drvdata object?

When pci_is_enabled(func) is true, wrap is retrieved from the sibling
function's drvdata:

cdns3_pci_probe() {
    ...
    if (pci_is_enabled(func)) {
        wrap = pci_get_drvdata(func);
    ...
}

If platform_device_register_full() fails later in the probe function, the
error path unconditionally calls kfree(wrap).

Because the sibling function still holds a pointer to this wrap object in
its drvdata, freeing it here can cause a use-after-free and potential
double-free when the sibling function is subsequently removed or accessed
(for example, when cdns3_pci_remove() dereferences wrap->devfn).

Should kfree(wrap) be skipped here if wrap was retrieved from the sibling
function rather than allocated in this probe call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012070310.3704130-1-make_ruc2021@163.com?part=1