Re: [PATCH v4 24/24] 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()` when reset controller registration fails, leading to a Use-After-Free.
--

--- Patch [24]: [PATCH v4 24/24] clk: mediatek: Add MT8189 ufs clock support ---
commit 73843c3d7b6ae3d2d34edc45116556bb9d3c1e8b
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.

> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-ufs.c

[ ... ]

> +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,
> +};

[ ... ]

> +static struct platform_driver clk_mt8189_ufs_drv = {
> +	.probe = mtk_clk_simple_probe,
> +	.remove = mtk_clk_simple_remove,
> +	.driver = {
> +		.name = "clk-mt8189-ufs",
> +		.of_match_table = of_match_clk_mt8189_ufs,
> +	},
> +};

[Severity: High]
This isn't a bug introduced by this patch, but since this new driver uses
mtk_clk_simple_probe() and registers a reset controller (rst_desc), it
exposes a pre-existing issue in the error handling path of
__mtk_clk_simple_probe().

If 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:
    ...
free_data:
    mtk_free_clk_data(clk_data);
}

Does this cleanup path leave a dangling pointer in the OF clock provider list?

It appears to free clk_data but fails to call of_clk_del_provider() to remove
the successfully registered clock provider.

If another driver attempts to look up a clock via the device tree after
probe failure, could of_clk_hw_onecell_get() dereference the freed clk_data
and cause a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-mt8189-clocks-system-base-v4-0-e356b813a64c@collabora.com?part=24