Re: [PATCH v3] usb: cdnsp: Add support for device-only configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The function `cdns_drd_host_on()` returns an uninitialized variable `ret` when `cdns->no_drd` is true.
- [High] Use-after-free and double-free in `cdnsp_pci_probe()` error path.
- [High] Concurrent probing of paired PCI functions leads to a NULL pointer dereference.
- [High] Partial unbind of PCI functions leaves the platform device running with disabled MMIO regions.
--

commit b3f808c677ea7f7e8a40cfd26c7b60a0f4d50887
Author: Pawel Laszczak <pawell@cadence.com>

usb: cdnsp: Add support for device-only configuration
    
This patch introduces support for the Cadence USBSSP (cdnsp) controller in
hardware configurations where the Dual-Role Device (DRD) register block is
not implemented or is inaccessible.

> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 432007cfe6954..992700479172f 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
[ ... ]
> @@ -93,7 +103,7 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  
>  	pci_set_master(pdev);
>  
> -	if (pci_is_enabled(func)) {
> +	if (func && pci_is_enabled(func)) {
>  		wrap = pci_get_drvdata(func);

This wasn't introduced by this patch, but does this code have a race condition
during concurrent probing? 

If both PCI functions are probed simultaneously (e.g., via sysfs bind), both
threads could see pci_is_enabled(func) as true before either has called
pci_set_drvdata(). 

This would cause pci_get_drvdata() to return NULL, leading to an immediate
NULL pointer dereference when accessing wrap->dev_res.

>  	} else {
>  		wrap = kzalloc_obj(*wrap);
[ ... ]
> @@ -158,6 +181,7 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  		plat_info.dma_mask = pdev->dma_mask;
>  		plat_info.data = &pdata;
>  		plat_info.size_data = sizeof(pdata);
> +		plat_info.properties = wrap->prop;
>  		wrap->devfn = pdev->devfn;
>  		/* register platform device */
>  		wrap->plat_dev = platform_device_register_full(&plat_info);
>  		if (IS_ERR(wrap->plat_dev)) {
>  			ret = PTR_ERR(wrap->plat_dev);
>  			kfree(wrap);

This wasn't introduced by this patch, but is it safe to unconditionally call
kfree(wrap) here if we're probing the second PCI function? 

Since the wrap structure was allocated and set as drvdata by the first
function, freeing it here could destroy the shared structure while the first
function is still actively bound and relying on its drvdata.

When the first function is eventually unbound, cdnsp_pci_remove() would
retrieve this dangling pointer, dereference it, and free it again.

>  			goto put_pci;
>  		}
>  	}
[ ... ]
> @@ -185,13 +209,17 @@ static void cdnsp_pci_remove(struct pci_dev *pdev)
>  	if (wrap->devfn == pdev->devfn)
>  		platform_device_unregister(wrap->plat_dev);

This wasn't introduced by this patch, but what happens if a user manually
unbinds the PCI function that didn't trigger the platform device registration?

It looks like the platform device would remain registered and active since
wrap->devfn != pdev->devfn, but the underlying PCI MMIO resources for this
function would be abruptly disabled. Could this cause a fatal bus fault on
subsequent accesses by the platform child drivers to the disabled BARs?

> -	if (!pci_is_enabled(func))
> +	if (!func || !pci_is_enabled(func))
>  		kfree(wrap);
>  
>  	pci_dev_put(func);
>  }
[ ... ]
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index 84fb38a5723a3..f87cf85cb97a1 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
[ ... ]
> @@ -181,6 +184,9 @@ int cdns_drd_host_on(struct cdns *cdns)
>  	u32 val, ready_bit;
>  	int ret;
>  
> +	if (cdns->no_drd)
> +		goto phy_set;
> +
>  	/* Enable host mode. */
>  	writel(OTGCMD_HOST_BUS_REQ | OTGCMD_OTG_DIS,
>  	       &cdns->otg_regs->cmd);
> @@ -197,6 +203,7 @@ int cdns_drd_host_on(struct cdns *cdns)
>  	if (ret)
>  		dev_err(cdns->dev, "timeout waiting for xhci_ready\n");
>  
> +phy_set:
>  	phy_set_mode(cdns->usb2_phy, PHY_MODE_USB_HOST);
>  	phy_set_mode(cdns->usb3_phy, PHY_MODE_USB_HOST);
>  	return ret;

If cdns->no_drd is true, we jump directly to phy_set, skipping the hardware
polling loop that assigns a value to ret. 

Does this cause the function to return uninitialized stack garbage in ret?

Callers checking for a non-zero return value might abort the host
initialization unexpectedly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-no_drd_config-v3-1-0614f5044721@cadence.com?part=1