Re: [PATCH v14 05/38] phy: rockchip: usbdp: Handle missing clock-names DT property gracefully

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

Pre-existing issues:
- [High] The exact NULL pointer dereference vulnerability fixed in `phy-rockchip-usbdp.c` exists in multiple other drivers which were overlooked in this patch.
- [High] Clock prepare/enable and reset assertion imbalance if probe fails when PHY is already in DP mode.
--

--- Patch [5]: [PATCH v14 05/38] phy: rockchip: usbdp: Handle missing clock-names DT property gracefully ---
commit 6c56ea962e4b5726f304742232f476db1aa9e0f2
Author: Sebastian Reichel <[email protected]>

phy: rockchip: usbdp: Handle missing clock-names DT property gracefully

This commit fixes a null pointer dereference in the phy clock initialization
when a non-compliant device tree specifies clocks but no clock-names.

Fixes: 2f70bbddeb45 ("phy: rockchip: add usbdp combo phy driver")
Reported-by: Sashiko <[email protected]>
Closes: https://sashiko.dev/#/message/20260619154349.071321F000E9%40smtp.kernel.org
Tested-by: Igor Paunovic <[email protected]> # Orange Pi 5 Plus

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 2845a012eafcc..3fc8222fcaec2 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -432,6 +432,9 @@ static int rk_udphy_clk_init(struct rk_udphy *udphy, struct device *dev)
>  
>  	/* used for configure phy reference clock frequency */
>  	for (i = 0; i < udphy->num_clks; i++) {
> +		if (!udphy->clks[i].id)
> +			continue;
> +

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
missing clock-names property gracefully here, should similar fixes be
applied to other drivers that use devm_clk_bulk_get_all() and check the id?

The same null pointer dereference can occur in these locations if
clock-names is missing from the device tree:

drivers/phy/rockchip/phy-rockchip-inno-usb2.c:rockchip_usb2phy_clk480m_register()
    if (!strncmp(rphy->clks[i].id, "phyclk", 6)) {

drivers/phy/rockchip/phy-rockchip-naneng-combphy.c:rockchip_combphy_parse_dt()
    if (!strncmp(priv->clks[i].id, "ref", 3)) {

drivers/pci/controller/dwc/pci-imx6.c:imx_setup_phy_mpll()
    if (strncmp(clks[i].id, "pcie_phy", 8) == 0)

drivers/pci/controller/dwc/pci-imx6.c:imx_pcie_probe()
    if (strncmp(imx_pcie->clks[i].id, "extref", 6) == 0)

drivers/spi/spi-rzv2h-rspi.c:rspi_probe()
    if (!strcmp(clks[i].id, rspi->info->tclk_name)) {

Could these drivers also crash on boot with a malformed device tree?

>  		if (!strncmp(udphy->clks[i].id, "refclk", 6)) {
>  			udphy->refclk = udphy->clks[i].clk;
>  			break;

[ ... ]

[Severity: High]
This is a pre-existing issue, but does probe failure lead to a resource
leak if the phy is already in dp mode?

If the phy is already active, rk_udphy_get_initial_status() prepares and
enables the clocks but intentionally skips rk_udphy_disable() to keep them
active:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_get_initial_status() {
    ...
    rk_udphy_disable(udphy);
}

If a subsequent step in probe fails, the function returns without cleaning
up those resources:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_probe() {
    ret = rk_udphy_get_initial_status(udphy);
    if (ret)
        return ret;
    ...
    if (ret)
        return ret;
}

Since devm_clk_bulk_get_all() only registers a devres action to call
clk_put(), wouldn't this permanently leak the enable counts and unasserted
resets on probe deferral?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy