Re: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

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

Pre-existing issues:
- [Critical] Missing bounds check on `count` leads to a severe stack buffer overflow when reading device tree properties.
- [High] System suspend causes a NULL pointer dereference or Use-After-Free when the driver uses the legacy device tree binding or fails parsing.
- [High] Duplicate indices in the `clock-indices` device tree property lead to a double-free and potential invalid pointer dereference on the error path.
--

--- Patch [1]: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path ---
commit 662987daf90805747f064d71045351ad4f076ebd
Author: Guangshuo Li <[email protected]>

clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

This commit fixes the error path of imx_lpcg_parse_clks_from_dt to properly
undo the runtime PM configurations. It ensures autosuspend is disabled and the
usage counter is correctly dropped on registration failure.

> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index f0cf6cf918992..fa79d52cbb36e 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -277,8 +277,9 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  		if (clk_hws[idx])
>  			imx_clk_lpcg_scu_unregister(clk_hws[idx]);
>  	}

[Severity: High]
This is a pre-existing issue, but does this teardown loop properly handle
duplicate indices in the clock-indices device tree property?

If the device tree contains duplicate values in clock-indices, the second
clock unconditionally overwrites the first in clk_hws[idx] during registration:

	clk_hws[idx] = imx_clk_lpcg_scu_dev(&pdev->dev, output_names[i],
					    parent_names[i], 0, base,
					    bit_offset[i], false);

If an error occurs on a subsequent clock registration, the teardown loop
iterates backward and calls imx_clk_lpcg_scu_unregister() on clk_hws[idx].
Because clk_hws[idx] is never nulled out after unregistration, when the loop
encounters the duplicate index again, could it call kfree() on the
already-freed pointer, leading to a double-free?

Furthermore, if the failed registration itself returned an ERR_PTR that
overwrote a duplicate index, would the loop pass the ERR_PTR to
imx_clk_lpcg_scu_unregister() and crash the system?

> -
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>  
>  	return ret;
>  }

While reviewing this code, I noticed a few other pre-existing issues in
the surrounding code.

[Severity: Critical]
This is a pre-existing issue, but does this function properly validate the
count retrieved from the device tree before using it to populate stack arrays?

imx_lpcg_parse_clks_from_dt() retrieves the number of elements into count:

	count = of_property_count_u32_elems(np, "clock-indices");

It then uses count to populate statically sized stack arrays (bit_offset,
parent_names, and output_names) of size IMX_LPCG_MAX_CLKS:

	ret = of_property_read_u32_array(np, "clock-indices", bit_offset,
					 count);
	...
	ret = of_clk_parent_fill(np, parent_names, count);
	...
	ret = of_property_read_string_array(np, "clock-output-names",
					    output_names, count);

If a device tree provides more than IMX_LPCG_MAX_CLKS elements, will this
overflow these stack allocations?

[Severity: High]
This is a pre-existing issue, but does system suspend cause a NULL pointer
dereference or Use-After-Free when the driver uses the legacy device tree
binding or fails parsing?

If the driver falls back to legacy device tree parsing, clocks are
registered using imx_clk_lpcg_scu() which passes dev = NULL. This leaves the
device drvdata unset:

drivers/clk/imx/clk-lpcg-scu.c:__imx_clk_lpcg_scu() {
	...
	if (dev)
		dev_set_drvdata(dev, clk);
	...
}

Alternatively, if imx_lpcg_parse_clks_from_dt() successfully registers one
clock but fails on a subsequent one, the error path unregisters and frees the
clock without clearing drvdata, leaving a dangling pointer.

When the system suspends, imx_clk_lpcg_scu_suspend() retrieves this NULL or
dangling pointer via dev_get_drvdata(dev) and dereferences it:

drivers/clk/imx/clk-lpcg-scu.c:imx_clk_lpcg_scu_suspend() {
	...
	if (!strncmp("hdmi_lpcg", clk_hw_get_name(&clk->hw), strlen("hdmi_lpcg")))
	...
}

Can this lead to a panic during system suspend on systems using the legacy
binding or when encountering a malformed DT?

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