Re: [PATCH v3 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 probe error path leads to a Use-After-Free on `clk_data`.
--

--- Patch [18]: [PATCH v3 18/18] clk: mediatek: Add MT8189 ufs clock support ---
commit 3f4a4ae78ea13a3a95925571f6bdf7f9cc1c1ea8
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 ufs clock support

Add support for the MT8189 ufs clock controller, which provides clock gate
control for Universal Flash Storage.

> 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 of_device_id of_match_clk_mt8189_ufs[] = {
> +	{ .compatible = "mediatek,mt8189-ufscfg-ao", .data = &ufscfg_ao_reg_mcd },
> +	{ .compatible = "mediatek,mt8189-ufscfg-pdn", .data = &ufscfg_pdn_reg_mcd },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_match_clk_mt8189_ufs);
> +
> +static struct platform_driver clk_mt8189_ufs_drv = {
> +	.probe = mtk_clk_simple_probe,

[Severity: High]
This is a pre-existing issue, but by using mtk_clk_simple_probe() with a
reset descriptor (ufscfg_ao_rst_desc and ufscfg_pdn_rst_desc), this driver
exposes a missing cleanup step in the error handling path.

If mtk_register_reset_controller_with_dev() fails inside
__mtk_clk_simple_probe(), it jumps to the unregister_clks label:

drivers/clk/mediatek/clk-mtk.c:__mtk_clk_simple_probe() {
    ...
        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);
        return r;
}

Since of_clk_add_hw_provider() was called earlier in the probe sequence,
jumping to unregister_clks frees the clk_data structure without calling
of_clk_del_provider().

Could this leave a dangling pointer in the OF clock provider list, resulting
in a use-after-free on subsequent clock lookups?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-mt8189-clocks-system-base-v3-0-8e6d99ab3ad2@collabora.com?part=18