Re: [PATCH v3 4/4] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing bounds check on `num_blocks` leads to buffer overflow and memory corruption.
- [High] Off-by-one error in `cpuhp_setup_state` failure path causes out-of-bounds array access and invalid IRQ free.
- [High] Missing CPU hotplug teardown callback causes unbalanced `enable_irq` warnings and use-after-unmap crashes on initialization failure.
--

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

clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

EN751627 is based on the 1004Kc which uses a different interrupt number
for each CPU timer. Support both this and the EN751221 which uses a
single percpu interrupt.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index ed750e39cc4fa..dea6dbafa16ea 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -107,7 +128,10 @@ static int cevt_init_cpu(uint cpu)
>  	clockevents_config_and_register(cd, econet_timer.freq_hz,
>  					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
>  
> -	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +	if (econet_timer.is_percpu)
> +		enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +	else
> +		enable_irq(cd->irq);

Will this cause unbalanced enable_irq() warnings when a CPU is brought back
online?

Since cpuhp_setup_state() provides NULL for the teardown callback, taking a
CPU offline leaves the IRQ depth at 0.

When brought back online, cevt_init_cpu() calls enable_irq() again, which
could trigger the IRQ subsystem's unbalanced enable warning.

Also, if cpuhp_setup_state() fails midway, the hotplug core attempts to roll
back CPUs but does nothing due to the NULL teardown callback. The driver then
proceeds to unmap MMIO memory and free IRQs.

Could this leave active clockevent devices registered that will crash the
system when their set_next_event callback accesses the unmapped memory?

[ ... ]

> @@ -148,9 +177,23 @@ static void __init cevt_init(struct device_node *np)
>  
>  static int __init timer_init(struct device_node *np)
>  {
> -	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
> +	int num_blocks = of_address_count(np);

[ ... ]

> @@ -160,7 +203,7 @@ static int __init timer_init(struct device_node *np)
>  
>  	econet_timer.freq_hz = clk_get_rate(clk);
>  
> -	for (int i = 0; i < num_blocks; i++) {
> +	for (i = 0; i < num_blocks; i++) {
>  		econet_timer.membase[i] = of_iomap(np, i);

Could this overflow the econet_timer.membase array?

The econet_timer.membase array is statically sized to ECONET_NUM_BLOCKS.

If a device tree specifies more address resources than ECONET_NUM_BLOCKS, this
loop will write out of bounds and corrupt adjacent memory like the irqs array.

Also, if num_blocks is less than required for the number of CPUs, will
cevt_dev_init() dereference unmapped NULL memory?

[ ... ]

> @@ -216,11 +270,20 @@ static int __init timer_init(struct device_node *np)
>  	return 0;
>  
>  out_irq_free:
> -	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
> +	for (; i >= 0; i--) {
> +		if (econet_timer.is_percpu) {
> +			free_percpu_irq(econet_timer.irqs[i], &econet_timer_pcpu);
> +		} else {
> +			free_irq(econet_timer.irqs[i], NULL);
> +		}
> +	}

Could this perform an out-of-bounds access or free an uninitialized IRQ?

After successfully requesting num_irqs interrupts in the loop above, the loop
variable i is equal to num_irqs.

If the subsequent cpuhp_setup_state() call fails, the code executes goto
out_irq_free without decrementing i.

If num_irqs equals ARRAY_SIZE(econet_timer.irqs), this will access
econet_timer.irqs[num_irqs], which is past the end of the array. Even if
within bounds, it calls free_irq() on an unrequested index.

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