Re: [PATCH v2 15/35] clk: mediatek: Add FHCTL support to common 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: Use-After-Free of FHCTL base on probe deferral / rebind
- [Medium] clk: mediatek: memory leak of FHCTL iomem base on probe failure or no matches
--

commit 8d87854263bcfad5cea06d0fd17b75e7b846a747
Author: Akari Tsuyukusa <akkun11.open@gmail.com>

clk: mediatek: Add FHCTL support to common probe/remove helpers

Extend __mtk_clk_simple_probe() and __mtk_clk_simple_remove() to support
FHCTL. This reduces boilerplate code in apmixedsys drivers with FHCTL
support.

> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> index 1172e76516c03..8fa81a8671ad0 100644
> --- a/drivers/clk/mediatek/clk-mtk.c
> +++ b/drivers/clk/mediatek/clk-mtk.c
[ ... ]
> @@ -525,8 +529,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>  	}
>  
>  	if (mcd->plls) {
> -		r = mtk_clk_register_plls(&pdev->dev, mcd->plls,
> +#if IS_ENABLED(CONFIG_COMMON_CLK_MEDIATEK_FHCTL)
> +		if (mcd->fhctl_node) {
> +			fhctl_parse_dt(mcd->fhctl_node, mcd->pllfhs,
> +				       mcd->num_pllfhs);
> +			r = mtk_clk_register_pllfhs(&pdev->dev, mcd->plls,
> +						    mcd->num_plls, mcd->pllfhs,
> +						    mcd->num_pllfhs, clk_data);

[Severity: Medium]
This problem wasn't introduced by this patch, but does calling
fhctl_parse_dt() here expose drivers to a memory leak of the fhctl iomem
base mapped via of_iomap() in fhctl_parse_dt()?

If mtk_clk_register_pllfhs() fails, the code jumps to the free_data label
in __mtk_clk_simple_probe(), bypassing unregister_plls. As a result, the
mapped i/o memory seems to be permanently leaked.

Additionally, if mtk_clk_register_pllfhs() fails to register the very first
pll, it skips its internal cleanup loop entirely, meaning the memory mapped
by fhctl_parse_dt() is never unmapped.

[Severity: High]
This problem wasn't introduced by this patch, but does the common
registration path expose fhctl-enabled drivers to a use-after-free during
probe deferral or device rebinds?

During a successful call, fhctl_parse_dt() mutates the static mcd->pllfhs
array by setting pllfh->state.fh_enable = 1 and storing the mapped i/o
pointer in pllfh->state.base.

If the probe later fails and the device is removed,
mtk_clk_unregister_pllfhs() calls iounmap() on the base but fails to clear
the fh_enable flag or the stale base pointer from the static array.

If the device is subsequently re-bound and fhctl_parse_dt() fails early
(leaving the static array unmodified), won't mtk_clk_register_pllfhs()
still see fh_enable == 1 from the previous probe and blindly dereference
the stale, unmapped state.base pointer?

> +		} else
> +#endif
> +			r = mtk_clk_register_plls(&pdev->dev, mcd->plls,
>  					  mcd->num_plls, clk_data);
> +
>  		if (r)
>  			goto free_data;

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