Re: [PATCH v6 3/5] clocksource/timer-econet-en751221: Init teardown on error if possible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The newly added error path for `cpuhp_setup_state()` failure creates a fatal use-after-unmap vulnerability by freeing IRQs and unmapping I/O memory without unregistering the active clockevent devices.
--

commit e1e0b36b8164978eb079f38d9547d743cebd0a97
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Init teardown on error if possible

This commit reorganizes the initialization process of the econet timer by
separating it into two phases. It adds teardown logic to safely roll back
resource allocations like memory mappings and IRQ requests if an error occurs
before the point of no return.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
[ ... ]
> @@ -187,22 +166,45 @@ static int __init timer_init(struct device_node *np)
[ ... ]
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				"clockevents/econet/timer:starting",
> +				cevt_init_cpu, NULL);
> +	if (ret < 0) {
> +		pr_err("%pOFn: cpuhp setup failed (%d)\n", np, ret);
> +		goto out_irq_free;
> +	}

[Severity: Critical]
When cpuhp_setup_state() executes the startup callback across online CPUs,
it registers the clockevent devices. If this fails on a subsequent CPU,
cpuhp_setup_state() aborts. Because a NULL teardown callback was provided,
the CPU hotplug core skips rollback and leaves the successfully registered
clockevent devices active on the earlier CPUs.

The error path here then frees the IRQs and unmaps the I/O memory. If the
timekeeping core attempts to handle a timer interrupt or program the next
event on the still-registered clockevent devices, will it dereference the
unmapped memory?

Should the active clockevent devices be unregistered, or a teardown callback
provided to cpuhp_setup_state(), before freeing these resources?

> +
> +	/* Point of no return, do not attempt to tear down after this. */
[ ... ]
> +out_irq_free:
> +	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
> +out_irq_mapping:
> +	irq_dispose_mapping(econet_timer.irq);
> +out_membase:
> +	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
> +		if (econet_timer.membase[i]) {
> +			iounmap(econet_timer.membase[i]);
> +			econet_timer.membase[i] = NULL;
> +		}
> +	}
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528140046.2897674-1-cjd@cjdns.fr?part=3