Re: [PATCH] clocksource/drivers/timer-imx-gpt: fix resource leak on init error paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clocksource: Use-After-Unmap of timer base by sched_clock on init error
- [Medium] clocksource: missing irq_dispose_mapping on init error path
--

commit e37f50884f58329bb1905901be3a80315d582d01
Author: Haotian Zhang <vulab@iscas.ac.cn>

clocksource/drivers/timer-imx-gpt: fix resource leak on init error paths

mxc_timer_init_dt() maps the timer registers with of_iomap() and obtains
the ipg and per clocks with of_clk_get_by_name(), but every error path
jumps to err_kfree, which only frees the imx_timer structure.  The
ioremap'd mapping and the clock references are therefore leaked when
irq_of_parse_and_map() or _mxc_timer_init() fails.

Unmap the I/O region and release the clocks on those error paths.

> diff --git a/drivers/clocksource/timer-imx-gpt.c b/drivers/clocksource/timer-imx-gpt.c
> index 8335bd7f8c6ce..b8005054b30ae 100644
> --- a/drivers/clocksource/timer-imx-gpt.c
> +++ b/drivers/clocksource/timer-imx-gpt.c
> @@ -441,7 +441,7 @@ static int __init mxc_timer_init_dt(struct device_node *np,  enum imx_gpt_type t
>  	imxtm->irq = irq_of_parse_and_map(np, 0);
>  	if (imxtm->irq <= 0) {
>  		ret = -EINVAL;
> -		goto err_kfree;
> +		goto err_unmap;
>  	}
>  
>  	imxtm->clk_ipg = of_clk_get_by_name(np, "ipg");
> @@ -455,12 +455,19 @@ static int __init mxc_timer_init_dt(struct device_node *np,  enum imx_gpt_type t
>  
>  	ret = _mxc_timer_init(imxtm);
>  	if (ret)
> -		goto err_kfree;
> +		goto err_clk_put;
>  
>  	initialized = 1;
>  
>  	return 0;
>  
> +err_clk_put:
> +	if (!IS_ERR_OR_NULL(imxtm->clk_per))
> +		clk_put(imxtm->clk_per);
> +	if (!IS_ERR_OR_NULL(imxtm->clk_ipg))
> +		clk_put(imxtm->clk_ipg);

[Severity: Medium]
Does this error path in mxc_timer_init_dt() leak the IRQ mapping allocated
earlier by irq_of_parse_and_map()?

If _mxc_timer_init() fails, the code jumps here to clean up the clocks and
the ioremap, but it does not call irq_dispose_mapping(imxtm->irq) to clean
up the IRQ mapping.

> +err_unmap:
> +	iounmap(imxtm->base);

[Severity: High]
Can this unmap in mxc_timer_init_dt() cause a fatal use-after-unmap page
fault in the core kernel?

If _mxc_timer_init() fails late in its execution, it might return an error
after mxc_clocksource_init() has already run successfully. For example:

drivers/clocksource/timer-imx-gpt.c:_mxc_timer_init()
    ret = mxc_clocksource_init(imxtm);
    if (ret)
        return ret;
    return mxc_clockevent_init(imxtm);

If mxc_clockevent_init() returns an error, mxc_clocksource_init() has
already called sched_clock_register() and registered the clocksource using
imxtm->base. Since there are no APIs to unregister the sched_clock or
clocksource, they will continue to access this memory from hot paths like
timekeeping. Unmapping it here leaves those subsystems reading from freed
memory.

>  err_kfree:
>  	kfree(imxtm);
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008173604.2733975-1-vulab@iscas.ac.cn?part=1