Re: [PATCH v2 09/18] clk: mediatek: Add MT8189 apmixedsys clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] FHCTL I/O memory mapped by fhctl_parse_dt() is leaked if mtk_clk_register_pllfhs() fails during probe.

Pre-existing issues:
- [High] Dangling I/O pointers in the static pllfhs array can lead to a use-after-free Data Abort on driver re-probe.
--

--- Patch [9]: [PATCH v2 09/18] clk: mediatek: Add MT8189 apmixedsys clock support ---
commit 5ceb4e2595f9e63ad996ab6c91c8b563477a9fc7
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 apmixedsys clock support

Add support for the MT8189 apmixedsys clock controller, which provides
PLLs generated from SoC 26m.

> diff --git a/drivers/clk/mediatek/clk-mt8189-apmixedsys.c b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> new file mode 100644
> index 0000000000000..0657a50c30d9f
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c

[ ... ]

> +static struct mtk_pllfh_data pllfhs[] = {
> +	FH(CLK_APMIXED_ARMPLL_LL, 0, 0x003C),
> +	FH(CLK_APMIXED_ARMPLL_BL, 1, 0x0050),
> +	FH(CLK_APMIXED_CCIPLL, 2, 0x0064),
> +	FH(CLK_APMIXED_MAINPLL, 3, 0x0078),
> +	FH(CLK_APMIXED_MMPLL, 4, 0x008C),
> +	FH(CLK_APMIXED_MFGPLL, 5, 0x00A0),
> +	FH(CLK_APMIXED_EMIPLL, 6, 0x00B4),
> +	FH(CLK_APMIXED_TVDPLL1, 7, 0x00C8),
> +	FH(CLK_APMIXED_TVDPLL2, 8, 0x00DC),
> +	FH(CLK_APMIXED_MSDCPLL, 9, 0x00F0),
> +	FH(CLK_APMIXED_UFSPLL, 10, 0x0104),
> +	FH(CLK_APMIXED_APUPLL, 11, 0x0118),
> +	FH(CLK_APMIXED_APUPLL2, 12, 0x012c),
> +};

[ ... ]

> +static int clk_mt8189_apmixed_probe(struct platform_device *pdev)
> +{
> +	int r;
> +	struct clk_hw_onecell_data *clk_data;
> +	struct device_node *node = pdev->dev.of_node;
> +	const u8 *fhctl_node = "mediatek,mt8189-fhctl";
> +
> +	clk_data = mtk_alloc_clk_data(ARRAY_SIZE(apmixed_plls));
> +	if (!clk_data)
> +		return -ENOMEM;
> +
> +	fhctl_parse_dt(fhctl_node, pllfhs, ARRAY_SIZE(pllfhs));
> +

[Severity: High]
This is a pre-existing issue, but does unbinding and re-probing this driver
create a use-after-free risk with the static pllfhs array?

When the driver is unbound, mtk_clk_unregister_pllfhs() unmaps the FHCTL
base but leaves the state.base pointer and state.fh_enable flag set in the
static array:

drivers/clk/mediatek/clk-pllfh.c:mtk_clk_unregister_pllfhs() {
        ...
        if (fhctl_base)
                iounmap(fhctl_base);
        ...
}

If the driver is then re-probed and fhctl_parse_dt() encounters an error
(like an of_iomap failure), it returns without clearing the static array.

When probe proceeds to mtk_clk_register_pllfhs(), will it see fh_enable == 1
and attempt to access the unmapped I/O memory left over in state.base?

> +	r = mtk_clk_register_pllfhs(&pdev->dev, apmixed_plls, ARRAY_SIZE(apmixed_plls),
> +				    pllfhs, ARRAY_SIZE(pllfhs), clk_data);
> +	if (r)
> +		goto free_apmixed_data;

[Severity: Medium]
If mtk_clk_register_pllfhs() fails here, does the error handling path leak
the I/O memory mapped by fhctl_parse_dt()?

The goto jumps directly to free_apmixed_data, completely bypassing the
unregister_plls label where the unmapping would normally happen.

> +
> +	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> +	if (r)
> +		goto unregister_plls;
> +
> +	platform_set_drvdata(pdev, clk_data);
> +
> +	return 0;
> +
> +unregister_plls:
> +	mtk_clk_unregister_pllfhs(apmixed_plls, ARRAY_SIZE(apmixed_plls), pllfhs,
> +				  ARRAY_SIZE(pllfhs), clk_data);
> +free_apmixed_data:
> +	mtk_free_clk_data(clk_data);
> +	return r;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-mt8189-clocks-system-base-v2-0-2926da3db6cf@collabora.com?part=9