Re: [PATCH v5 25/25] clk: mediatek: Add MT8189 ufs clock support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing `of_clk_del_provider()` on the error path in `__mtk_clk_simple_probe()` leads to Use-After-Free.
--

--- Patch [25]: [PATCH v5 25/25] clk: mediatek: Add MT8189 ufs clock support ---
commit b3bac535ba9ad99268bbb264f23284ef21189814
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 ufs clock support

This commit adds support for the MT8189 ufs clock controller. It provides
clock gate control and reset capabilities for Universal Flash Storage on the
MT8189 SoC.

> diff --git a/drivers/clk/mediatek/clk-mt8189-ufs.c b/drivers/clk/mediatek/clk-mt8189-ufs.c
> new file mode 100644
> index 0000000000000..85afab04420f8
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-ufs.c

[ ... ]

> +static struct platform_driver clk_mt8189_ufs_drv = {
> +	.probe = mtk_clk_simple_probe,

[Severity: High]
This is a pre-existing issue, but does the error path in
__mtk_clk_simple_probe() leave a dangling pointer in the OF clock provider
list if reset controller registration fails?

When mtk_register_reset_controller_with_dev() fails, the code jumps to
unregister_clks:

drivers/clk/mediatek/clk-mtk.c:__mtk_clk_simple_probe() {
    ...
	if (mcd->rst_desc) {
		r = mtk_register_reset_controller_with_dev(&pdev->dev,
							   mcd->rst_desc);
		if (r)
			goto unregister_clks;
	}
    ...
unregister_clks:
	if (mcd->clks)
		mtk_clk_unregister_gates(mcd->clks, mcd->num_clks, clk_data);
...
free_data:
	mtk_free_clk_data(clk_data);
}

The unregister_clks error path frees clk_data but appears to miss calling
of_clk_del_provider() if of_clk_add_hw_provider() had already succeeded. 

Could this cause a use-after-free if a subsequent clock lookup invokes
of_clk_hw_onecell_get() with the freed clk_data pointer? Since this new
driver registers a reset controller and uses this probe function, it might
be exposed to this vulnerable error path if reset registration fails.

> +	.remove = mtk_clk_simple_remove,
> +	.driver = {
> +		.name = "clk-mt8189-ufs",
> +		.of_match_table = of_match_clk_mt8189_ufs,
> +	},
> +};
> +module_platform_driver(clk_mt8189_ufs_drv);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-mt8189-clocks-system-base-v5-0-ac4410697cc2@collabora.com?part=25