Re: [PATCH v4 2/2] 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] Uninitialized variable returned in cdns_drd_host_on()
- [High] Use-After-Free due to improper error handling in cdnsp_pci_probe()
- [High] Use-After-Free via sysfs due to failure to unregister usb_role_switch on cdns_core_init_role() error
- [High] Race condition in cdnsp_pci_probe() leading to NULL pointer dereference under asynchronous probing
--

commit b729274ea9c34ef88a5c2a918206d853477c97a2
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. It adjusts BAR indexing, adds a no_drd
flag, and skips DRD-specific initializations if no_drd is true.

> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> index 3fe3109a3688a..86c963a072dba 100644
> --- a/drivers/usb/cdns3/cdns3-plat.c
> +++ b/drivers/usb/cdns3/cdns3-plat.c
> @@ -81,6 +81,7 @@ static int cdns3_plat_probe(struct platform_device *pdev)
>  	if (cdns->pdata && cdns->pdata->override_apb_timeout)
>  		cdns->override_apb_timeout = cdns->pdata->override_apb_timeout;
>  
> +	cdns->no_drd = device_property_read_bool(dev, "no_drd");
>  	platform_set_drvdata(pdev, cdns);

This isn't a bug introduced by this patch, but does the error path in this
probe function leave a dangling role switch?

In cdns3_plat_probe(), cdns_init() registers a USB role switch which stores
a pointer to the devm-allocated cdns structure as its driver data.

If cdns_core_init_role() fails later in the probe:

cdns3_plat_probe()
    ret = cdns_init(cdns);
    ...
    ret = cdns_core_init_role(cdns);
    if (ret)
        goto err_cdns_init;

err_cdns_init:
    set_phy_power_off(cdns);
    ...

Is it possible the role switch remains alive and accessible via sysfs
without being unregistered, but the underlying cdns structure gets freed by
devres, leading to a use-after-free?

[ ... ]
> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 432007cfe6954..feb9162298709 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
[ ... ]
> @@ -93,7 +104,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 isn't a bug introduced by this patch, but could this race if
asynchronous probing is enabled?

If the first function is probed concurrently, it calls
pcim_enable_device(), which makes pci_is_enabled(func) evaluate to true
for the second function. 

If the second function reaches here before the first function calls
pci_set_drvdata() at the end of its probe, will pci_get_drvdata() return a
NULL pointer?

If so, could this lead to a NULL pointer dereference immediately following
this block when it populates resources via res = wrap->dev_res?

[ ... ]
> @@ -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 isn't a bug introduced by this patch, but does the error handling here
risk a double-free?

If the second PCI function is being probed and
platform_device_register_full() fails, the existing error handling does:

cdnsp_pci_probe()
    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;
    }

Since wrap was retrieved from the first function's driver data via
pci_get_drvdata(), the first function still holds this pointer. When the
first function is eventually unbound, will cdnsp_pci_remove() attempt to
use and free it again, resulting in a use-after-free?

[ ... ]
> 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;
> +

Does this jump bypass the initialization of the return variable?

When no_drd is true, the execution jumps to the phy_set label, bypassing
the block where ret is populated.

[ ... ]
> @@ -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;

When the function finishes and returns ret, will it return an uninitialized
value from the stack, potentially causing host initialization to fail
unpredictably?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-b4-no_drd_config-v4-0-7b4e5dc96f81@cadence.com?part=2