Re: [PATCH v5 2/2] usb: cdnsp: Add support for device-only configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized variable `ret` is returned in `cdns_drd_host_on()` when `cdns->no_drd` is true.
- [High] In `cdnsp_pci_probe()`, a failed `platform_device_register_full()` leads to a Use-After-Free and potential Double Free via a dangling pointer in the sibling PCI device's `drvdata`.
--

commit 3bccaf516e61eb4462bbd76ec7e121917dbcc00b
Author: Pawel Laszczak <pawell@cadence.com>
usb: cdnsp: Add support for device-only configuration

This patch introduces support for the Cadence USBSSP controller in hardware
configurations where the Dual-Role Device (DRD) register block is not
implemented. In these configurations, the driver operates exclusively in a
fixed mode and uses 32-bit addressing instead of 64-bit addressing.

> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
[ ... ]
> @@ -158,6 +182,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);

This is an existing issue in the code modified here, but if
platform_device_register_full() fails just below this hunk:

        wrap->plat_dev = platform_device_register_full(&plat_info);
        if (IS_ERR(wrap->plat_dev)) {
                ret = PTR_ERR(wrap->plat_dev);
                kfree(wrap);
                goto put_pci;
        }

When the second PCI function is probed and pci_is_enabled(func) is true,
wrap is retrieved from the first function's drvdata. Does calling kfree(wrap)
in this error path leave a dangling pointer in the first device's drvdata?
If so, could this lead to a use-after-free and a double free later when
cdnsp_pci_remove() is called?

> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> --- 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;
> +

When cdns->no_drd is true, this jumps to phy_set, bypassing the assignment
to ret. Since ret is declared without initialization, could returning it at
the end of the function propagate a non-zero garbage value? This might cause
spurious host mode initialization failures.

>  	/* 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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-b4-no_drd_config-v5-0-1c633a4b9a29@cadence.com?part=2