Re: [PATCH v2 14/35] clk: mediatek: Add child-device population to probe/remove helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: mediatek: NULL deref in __mtk_clk_simple_remove() without id_table fallback
- [High] clk: mediatek: Use-after-free of clk in devm notifier release
--

commit 4b72b775dd71cd9f8aca52f2f386291f6722da4e
Author: Akari Tsuyukusa <akkun11.open@gmail.com>

clk: mediatek: Add child-device population to probe/remove helpers
    
MediaTek audio clock drivers call devm_of_platform_populate() after
mtk_clk_simple_probe(). Extend __mtk_clk_simple_probe() to run
of_platform_populate() when the new 'populate_children' flag is set,
reducing boilerplate.

> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> index e06cd40d995a9..1172e76516c03 100644
> --- a/drivers/clk/mediatek/clk-mtk.c
> +++ b/drivers/clk/mediatek/clk-mtk.c
[ ... ]
> @@ -603,7 +604,16 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>  		r = mtk_register_reset_controller_with_dev(&pdev->dev,
>  							   mcd->rst_desc);
>  		if (r)
> -			goto unregister_clks;
> +			goto unregister_provider;
> +	}
> +
> +	if (mcd->populate_children) {
> +		r = of_platform_populate(pdev->dev.of_node, NULL, NULL,
> +					 &pdev->dev);
> +		if (r) {
> +			of_platform_depopulate(&pdev->dev);
> +			goto unregister_provider;

[Severity: High]
Does this new error path cause a use-after-free on the clk?

If __mtk_clk_simple_probe() has previously registered a rate-change
notifier on a struct clk using devm_clk_notifier_register() (via
mcd->clk_notifier_func), the notifier's lifetime is tied to the
pdev->dev devres list.

When taking this goto unregister_provider error path, the clocks are
manually unregistered via the cleanup labels, for example by calling
mtk_clk_unregister_muxes() which calls clk_hw_unregister() and then
clk_unregister().

clk_unregister() frees the struct clk pointer, but does not remove its
associated notifiers from clk_notifier_list.

Later, when the driver core invokes devres_release_all() for the device,
it calls devm_clk_notifier_release(), which invokes
clk_notifier_unregister() with the now-freed struct clk pointer, resulting
in a use-after-free.

This use-after-free also appears to occur during driver unbind in
__mtk_clk_simple_remove().

[ ... ]
> @@ -658,6 +671,9 @@ static void __mtk_clk_simple_remove(struct platform_device *pdev,
>  	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
>  	const struct mtk_clk_desc *mcd = device_get_match_data(&pdev->dev);
>  
> +	if (mcd->populate_children)

[Severity: High]
Can this dereference a NULL mcd pointer during driver removal?

Many MediaTek clock drivers register as platform drivers using an
id_table without an of_match_table. While __mtk_clk_simple_probe() handles
this by safely falling back to platform_get_device_id() when
device_get_match_data() returns NULL, __mtk_clk_simple_remove() lacks
this fallback.

When the driver is unbound, for instance via sysfs or module unload,
device_get_match_data() will return NULL, causing mcd->populate_children
to crash.

> +		of_platform_depopulate(&pdev->dev);
> +
>  	of_clk_del_provider(node);
>  	if (mcd->clks)
>  		mtk_clk_unregister_gates(mcd->clks, mcd->num_clks, clk_data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013151734.3177690-1-akkun11.open@gmail.com?part=14