Re: [PATCH v5 16/25] clk: mediatek: Add MT8189 apmixedsys clock support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The global static pllfhs array retains stale state across unbind/rebind cycles, leading to a Use-After-Free.
- [Medium] The FHCTL I/O memory mapping is leaked if mtk_clk_register_pllfhs() fails during probe.

Pre-existing issues:
- [High] The ssc_rate variable in fhctl_parse_dt() is used uninitialized if of_property_read_u32_index() fails.
--

--- Patch [16]: [PATCH v5 16/25] clk: mediatek: Add MT8189 apmixedsys clock support ---
commit c8fa878972c7f66453b67a7a8d8a6009e6ea57c9
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
> index 0000000000000..a50a8979b8495
> --- /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),

[Severity: High]
Does this global static array retain stale state across driver unbind and rebind
cycles?

fhctl_parse_dt() maps the FHCTL device node into memory and stores the pointer
in this static array, setting state.fh_enable = 1.

On driver unbind, mtk_clk_unregister_pllfhs() calls iounmap() on this pointer
but does not clear state.fh_enable or state.base.

If the driver is later re-bound and fhctl_parse_dt() fails (for example, if
of_iomap() fails), the array retains the old values.

When mtk_clk_register_pllfhs() executes, it will see fh_enable == 1 and attempt
to access the previously unmapped memory via pllfh_data->state.base, causing an
I/O use-after-free.

[ ... ]
> +static int clk_mt8189_apmixed_probe(struct platform_device *pdev)
> +{
> +	const u8 *fhctl_node = "mediatek,mt8189-fhctl";
> +	struct device_node *node = pdev->dev.of_node;
> +	struct clk_hw_onecell_data *clk_data;
> +	int r;
> +
> +	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 I noticed that fhctl_parse_dt() can use an
uninitialized variable.

In drivers/clk/mediatek/clk-pllfh.c:fhctl_parse_dt():

	u32 num_clocks, pll_id, ssc_rate;
	...
		of_property_read_u32_index(node,
					   "mediatek,hopping-ssc-percent",
					   i, &ssc_rate);

		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll_id);
		if (!pllfh)
			continue;

		pllfh->state.fh_enable = 1;
		pllfh->state.ssc_rate = ssc_rate;

If the "mediatek,hopping-ssc-percent" property is missing from the device tree,
of_property_read_u32_index() returns an error and leaves ssc_rate untouched.
The uninitialized stack garbage is then unconditionally stored in
pllfh->state.ssc_rate.

Since this value is later used to program hardware registers, could it cause
erratic clock frequencies or system instability?

> +
> +	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]
Does this error path leak the FHCTL I/O memory mapping?

fhctl_parse_dt() maps the FHCTL device node into memory. If
mtk_clk_register_pllfhs() fails, the driver jumps directly to
free_apmixed_data and exits without unmapping the FHCTL memory.

Because this can happen on probe deferral, repeated failures could leak
virtual memory space.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-mt8189-clocks-system-base-v5-0-ac4410697cc2@collabora.com?part=16