Re: [PATCH v2 25/35] clk: mediatek: mt7988-apmixed: Switch to common probe/remove helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: mtk: out-of-bounds array write in mtk_clk_register functions
--

commit 9e2f22353c17eb582f6f6eadd7b30d78eb6c4de2
Author: Akari Tsuyukusa <akkun11.open@gmail.com>

clk: mediatek: mt7988-apmixed: Switch to common probe/remove helpers

The MT7988 apmixedsys driver used a custom probe to register PLLs but
had no remove callback. With PLL registration and unregistration
handled by the common helpers, the driver can use mtk_clk_simple_probe()
and mtk_clk_simple_remove().

> diff --git a/drivers/clk/mediatek/clk-mt7988-apmixed.c b/drivers/clk/mediatek/clk-mt7988-apmixed.c
> index 24f4e7b87a312..f7bb00e7d17a1 100644
> --- a/drivers/clk/mediatek/clk-mt7988-apmixed.c
> +++ b/drivers/clk/mediatek/clk-mt7988-apmixed.c
[ ... ]
> -free_apmixed_data:
> -	mtk_free_clk_data(clk_data);
> -	return r;
> -}
> -
>  static struct platform_driver clk_mt7988_apmixed_drv = {
> -	.probe = clk_mt7988_apmixed_probe,
> +	.probe = mtk_clk_simple_probe,
> +	.remove = mtk_clk_simple_remove,

[Severity: High]
While this is not a regression for mt7988-apmixedsys because its IDs are
densely packed (0..11), does migrating to mtk_clk_simple_probe cause an
out-of-bounds array write for other drivers migrated in this series?

Looking at __mtk_clk_simple_probe() in clk-mtk.c, it allocates clk_data based
on the total count of clock elements:

drivers/clk/mediatek/clk-mtk.c:__mtk_clk_simple_probe() {
	...
	num_clks = mcd->num_clks + mcd->num_composite_clks;
	num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
	num_clks += mcd->num_mux_clks + mcd->num_divider_clks;
	num_clks += mcd->num_cpumuxes + mcd->num_plls;

	clk_data = mtk_alloc_clk_data(num_clks);
	...
}

For drivers with sparse clock IDs like mt2701 topckgen, the total element
count (149) is smaller than the maximum ID (CLK_TOP_AXISEL_D4 is 158).
When mtk_clk_register_factors() processes this, will it write out-of-bounds
into adjacent slab memory?

drivers/clk/mediatek/clk-mtk.c:mtk_clk_register_factors() {
	...
		clk_data->hws[ff->id] = hw;
	...
}

Could this cause a kernel panic during boot when loading other Mediatek clock
drivers with sparse IDs (like mt2701 and mt6779) migrated in this series?

>  	.driver = {
>  		.name = "clk-mt7988-apmixed",
>  		.of_match_table = of_match_clk_mt7988_apmixed,
>  	},
>  };
> -builtin_platform_driver(clk_mt7988_apmixed_drv);
> +module_platform_driver(clk_mt7988_apmixed_drv);
>  
>  MODULE_DESCRIPTION("MediaTek MT7988 apmixedsys clocks driver");
>  MODULE_LICENSE("GPL");

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