Re: [PATCH v2 16/35] clk: mediatek: mt2701: Switch to common probe/remove helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] clk: mediatek: heap buffer overflow in mtk_clk_simple_probe() due to sparse clock IDs
- [High] clk: mediatek: UAF from __initconst cpu_muxes during deferred probe
--

commit 659dcd3d07d9476de418d05bc84f3a19e38f1127
Author: Akari Tsuyukusa <akkun11.open@gmail.com>

clk: mediatek: mt2701: Switch to common probe/remove helpers

The MT2701 topckgen and pericfg drivers register clock types already
supported by the common helpers, so their custom initialization
routines can be replaced with struct mtk_clk_desc.

> diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
> index 710c4f2f9f304..5ea0c4acaf1be 100644
> --- a/drivers/clk/mediatek/clk-mt2701.c
> +++ b/drivers/clk/mediatek/clk-mt2701.c

[ ... ]

> @@ -655,38 +656,19 @@ static const struct mtk_gate top_clks[] = {
>  		28),
>  };
>  
> -static int mtk_topckgen_init(struct platform_device *pdev)
> -{

[ ... ]

> -	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> -}
> +static const struct mtk_clk_desc topck_desc = {
> +	.clks = top_clks,
> +	.num_clks = ARRAY_SIZE(top_clks),

[Severity: Critical]
Does this code cause a heap buffer overflow regression during probe?

Looking at __mtk_clk_simple_probe() in drivers/clk/mediatek/clk-mtk.c, it
allocates clk_data based on the sum of all element counts:

__mtk_clk_simple_probe()
    num_clks = mcd->num_clks + mcd->num_composite_clks;
    num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
    ...
    clk_data = mtk_alloc_clk_data(num_clks);

Since the device tree clock IDs are sparse, the maximum clock ID can be much
larger than the total number of registered clocks. 

When mtk_clk_register_gates() assigns the hardware clock to the index
matching its ID:

mtk_clk_register_gates()
    if (!IS_ERR_OR_NULL(clk_data->hws[cg->id])) {

won't it write out of bounds of the undersized array?

[ ... ]

> @@ -725,81 +707,21 @@ static const struct mtk_fixed_factor infra_fixed_divs[] = {
>  static u16 infrasys_rst_ofs[] = { 0x30, 0x34, };
>  static u16 pericfg_rst_ofs[] = { 0x0, 0x4, };
>  
> -static const struct mtk_clk_rst_desc clk_rst_desc[] = {

[ ... ]

> -	mtk_register_reset_controller_with_dev(&pdev->dev, &clk_rst_desc[0]);
> -
> -	return 0;
> -}
> +static const struct mtk_clk_desc infracfg_desc = {
> +	.clks = infra_clks,
> +	.num_clks = ARRAY_SIZE(infra_clks),
> +	.factor_clks = infra_fixed_divs,
> +	.num_factor_clks = ARRAY_SIZE(infra_fixed_divs),
> +	.cpumuxes = cpu_muxes,

[Severity: High]
Does this create a use-after-free regression for cpu_muxes?

The cpu_muxes array is annotated with __initconst. Previously it was used in
an early init callback, but this patch passes it to the standard platform
driver probe function mtk_clk_simple_probe() via the infracfg_desc.cpumuxes
pointer.

Since mtk_clk_simple_probe() can run during deferred probe or upon manual
sysfs bind after the module initialization sections have been freed, will the
probe function access the freed cpu_muxes array?

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