Re: [PATCH v2 18/18] 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] Missing `of_clk_del_provider()` in the error path of `__mtk_clk_simple_probe()` leaves a dangling clock provider pointer, leading to a potential use-after-free.
--

--- Patch [18]: [PATCH v2 18/18] clk: mediatek: Add MT8189 ufs clock support ---
commit 9858e88b79ed61ffe0a3b2d09755d7d4f94f084e
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, providing clock
gate control for Universal Flash Storage. It introduces a new driver handling
clock sources, dividers, and gates specific to the MT8189 platform.

> 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 const struct mtk_clk_rst_desc ufscfg_ao_rst_desc = {
> +	.version = MTK_RST_SET_CLR,
> +	.rst_bank_ofs = ufscfg_ao_rst_ofs,
> +	.rst_bank_nr = ARRAY_SIZE(ufscfg_ao_rst_ofs),
> +	.rst_idx_map = ufscfg_ao_rst_idx_map,
> +	.rst_idx_map_nr = ARRAY_SIZE(ufscfg_ao_rst_idx_map),
> +};
> +
> +static const struct mtk_clk_desc ufscfg_ao_reg_mcd = {
> +	.clks = ufscfg_ao_reg_clks,
> +	.num_clks = ARRAY_SIZE(ufscfg_ao_reg_clks),
> +	.rst_desc = &ufscfg_ao_rst_desc,

[Severity: High]
This is a pre-existing issue, but does setting .rst_desc here expose a
potential use-after-free in the probe error path?

Looking at __mtk_clk_simple_probe() in drivers/clk/mediatek/clk-mtk.c:

	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
	if (r)
		goto unregister_clks;

	platform_set_drvdata(pdev, clk_data);

	if (mcd->rst_desc) {
		r = mtk_register_reset_controller_with_dev(&pdev->dev,
							   mcd->rst_desc);
		if (r)
			goto unregister_clks;
	}

If mtk_register_reset_controller_with_dev() fails, the code jumps to
unregister_clks, which frees clk_data without calling of_clk_del_provider().
Any subsequent attempt to resolve a clock from this node could trigger a
use-after-free on the freed clock provider pointer.

Should __mtk_clk_simple_probe() be fixed to cleanly unregister the provider
before this new driver uses .rst_desc?

> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-mt8189-clocks-system-base-v2-0-2926da3db6cf@collabora.com?part=18