Re: [PATCH 4/8] phy: tegra: xusb: Use devm_clk_get_optional to fetch USB2 tracking clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Double free of pad allocations on `tegra_xusb_pad_init()` failure due to incorrect device cleanup.
- [High] Kernel panic due to NULL pointer dereference in `tegra_xusb_port_release()` when port initialization fails.
--

commit 9a7eb66c6206c1b8eab420ee00c7a1d273a708fe
Author: Sing-Han Chen <singhanc@nvidia.com>
Subject: phy: tegra: xusb: Use devm_clk_get_optional to fetch USB2 tracking clock

This patch switches the USB2 tracking clock retrieval to use
devm_clk_get_optional. This allows the probe sequence to proceed safely
when the clock is omitted from the devicetree.

> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index 60156aea27071..800be0cbb3f9f 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -1045,7 +1045,7 @@ tegra186_usb2_pad_probe(struct tegra_xusb_padctl *padctl,
>  		goto out;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the error path for
tegra_xusb_pad_init() just above this change cause a double free?

Looking at tegra186_usb2_pad_probe(), if tegra_xusb_pad_init() fails, it
calls kfree(usb2).

However, inside tegra_xusb_pad_init(), an error causes it to jump to the
unregister label and call device_unregister(&pad->dev). This drops the
device reference and invokes the release callback tegra_xusb_pad_release(),
which frees the pad object.

When tegra_xusb_pad_init() returns the error back to the probe function, the
explicit kfree(usb2) is executed. Could this lead to memory corruption when
initialization fails?

>  
> -	priv->usb2_trk_clk = devm_clk_get(&pad->dev, "trk");
> +	priv->usb2_trk_clk = devm_clk_get_optional(&pad->dev, "trk");
>  	if (IS_ERR(priv->usb2_trk_clk)) {
>  		err = PTR_ERR(priv->usb2_trk_clk);
>  		dev_dbg(&pad->dev, "failed to get usb2 trk clock: %d\n", err);

[ ... ]

[Severity: High]
This is another pre-existing issue in a related file, but does
tegra_xusb_port_release() trigger a NULL pointer dereference on
initialization failure?

In drivers/phy/tegra/xusb.c:tegra_xusb_add_usb2_port(), the port is
allocated via kzalloc() and then initialized:

    err = tegra_xusb_port_init(&usb2->base, padctl, np, "usb2", index);
    if (err < 0)
        goto out;

    usb2->base.ops = padctl->soc->ports.usb2.ops;

Notice that the ops pointer is assigned after initialization completes.

If tegra_xusb_port_init() fails, it drops the device refcount and calls
tegra_xusb_port_release(). That release function attempts to evaluate the
ops pointer:

    if (port->ops->release)
        port->ops->release(port);

Since port->ops is not yet assigned during the init error path, it will
still be NULL from the kzalloc() allocation. Could this cause a panic when
initialization fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629093406.1118594-1-waynec@nvidia.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy