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

[PATCH v3] 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.

Route the error paths of cdns3_pci_probe() through a put_pci label and
drop the reference there.

cdns3_pci_remove() needs the reference dropped too, and it is missing
a NULL check while at it. cdns3_get_second_fun() can return NULL, and
pci_is_enabled() reads pci_dev->enable_cnt without checking its
argument.

The same error path also frees the wrong object. When the second
function has already been probed, wrap is its drvdata, obtained with
pci_get_drvdata(), and it is shared. The second function keeps
pointing at it, and cdns3_pci_remove() dereferences it when that
function goes away.  Freeing it here leaves that function with a
dangling drvdata.

Both branches that assign wrap are guarded by the same
pci_is_enabled() test as the registration block that can fail, so the
wrap freed there is always the one taken from the sibling function.
Track whether wrap was allocated here and only free it in that case.

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 v3:
- Do not free a wrap that was taken from the second function's drvdata.
  The free in that error path can only ever run on the shared object, so
  it left the sibling function with a dangling drvdata. Reported by
  Sashiko. cdnsp-pci.c has the same construct.
- Keep the rest of v2.
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 | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
index fd06a3d8e638..31089eb9ca29 100644
--- a/drivers/usb/cdns3/cdns3-pci-wrap.c
+++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
@@ -72,6 +72,7 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
 	struct cdns3_wrap *wrap;
 	struct resource *res;
 	struct pci_dev *func;
+	bool own_wrap;
 	int err;
 
 	/*
@@ -99,12 +100,17 @@ 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;
 		}
+		own_wrap = false;
 	} else {
 		wrap = kzalloc_obj(*wrap);
-		if (!wrap)
-			return -ENOMEM;
+		if (!wrap) {
+			err = -ENOMEM;
+			goto put_pci;
+		}
+		own_wrap = true;
 	}
 
 	res = wrap->dev_res;
@@ -164,12 +170,15 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
 		wrap->plat_dev = platform_device_register_full(&plat_info);
 		if (IS_ERR(wrap->plat_dev)) {
 			err = PTR_ERR(wrap->plat_dev);
-			kfree(wrap);
-			return err;
+			if (own_wrap)
+				kfree(wrap);
+			goto put_pci;
 		}
 	}
 
 	pci_set_drvdata(pdev, wrap);
+put_pci:
+	pci_dev_put(func);
 	return err;
 }
 
@@ -184,8 +193,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 v3] usb: cdns3: Fix PCI device reference leak in cdns3_pci_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: cdns3: TOCTOU race in probe corrupts shared wrap state
- [High] pci: UAF of MMIO resources on partial unbind of compound device
- [Medium] usb: cdns3: PCI device reference leak on pcim_enable_device() failure
- [Medium] pci: memory leak of cdns3_wrap on concurrent unbind due to devres timing
--

commit 8da98a756feb810e31a39db878f286ba953f3a5d
Author: Ma Ke <make_ruc2021@163.com>
Date:   Tue Oct 13 02:08:27 2026 +0000

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

This commit addresses a reference leak of the sibling PCI function in
cdns3_pci_probe() and cdns3_pci_remove() by ensuring pci_dev_put() is
called. It also tracks the allocation of the shared cdns3_wrap structure
to avoid freeing a dangling pointer when the second function fails probe.

> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
> index fd06a3d8e638f..31089eb9ca29f 100644
> --- a/drivers/usb/cdns3/cdns3-pci-wrap.c
> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
> @@ -72,6 +72,7 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
>  	struct cdns3_wrap *wrap;
>  	struct resource *res;
>  	struct pci_dev *func;
> +	bool own_wrap;
>  	int err;

[Severity: Medium]
Does this code still leak the func reference if pcim_enable_device() fails?

Earlier in cdns3_pci_probe(), before the changes in this patch take effect,
we have this code:

	func = cdns3_get_second_fun(pdev);
	if (unlikely(!func))
		return -EINVAL;

	err = pcim_enable_device(pdev);
	if (err) {
		dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
		return err;
	}

If pcim_enable_device() fails here, we return immediately without dropping
the func reference acquired by cdns3_get_second_fun(). Should this error path
be routed to the new put_pci label as well?

[ ... ]

> @@ -99,12 +100,17 @@ 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;
>  		}

[Severity: High]
Can a Time-of-Check to Time-of-Use race happen here during concurrent probe?

If both function 0 and function 1 are probed concurrently (e.g., via sysfs
or async boot probing):

1. CPU0 enables device 0.

2. CPU1 enables device 1, sees device 0 is enabled, but wrap is NULL because
   CPU0 hasn't set drvdata yet. CPU1 defers and keeps device 1 enabled.

3. CPU0 sees device 1 enabled, assumes wrap is initialized, and registers a
   platform device with incomplete resources.

4. CPU1 retries later, gets the wrap structure from CPU0, and proceeds to
   register a second platform device.

This second platform device registration overwrites wrap->plat_dev and
wrap->devfn. Since the remove method relies on wrap->devfn to unregister the
platform device, does this permanently leak the first platform device?

[ ... ]

> @@ -184,8 +193,10 @@ static void cdns3_pci_remove(struct pci_dev *pdev)
>  	if (wrap->devfn == pdev->devfn)
>  		platform_device_unregister(wrap->plat_dev);

[Severity: High]
Does this leave the platform device accessing disabled MMIO resources on a
partial unbind?

The driver registers a single platform device for both functions, assigning it
the resources (BARs) of both. If the function that was probed first is unbound
via sysfs, wrap->devfn != pdev->devfn (since only the second probed function
records its devfn). The unregister is skipped.

When cdns3_pci_remove() completes, pcim_release() will disable the unbound
function's memory decoding. The platform device remains registered and its
driver will continue accessing disabled MMIO regions. Can this lead to master
aborts or system hangs?

> -	if (!pci_is_enabled(func))
> +	if (!func || !pci_is_enabled(func))
>  		kfree(wrap);

[Severity: Medium]
Will this skip freeing the shared cdns3_wrap on concurrent unbind?

If both functions are unbound concurrently via sysfs, they will both enter
cdns3_pci_remove() at the same time. Since device enablement is managed by
devres (via pcim_enable_device), devres_release_all() only decrements
enable_cnt after the remove function returns.

When both threads check pci_is_enabled(func) for their sibling, both will see
it as true (because neither sibling's devres cleanup has run yet). Both will
skip the kfree(wrap) call, permanently leaking the structure.

> +
> +	pci_dev_put(func);
>  }

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