Re: [PATCH v11 5/5] clocksource/timer-econet-en751221: Do not use NR_CPUS for resource array size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] timer: unhandled NULL return from reg_ctl() causes panic in cevt_is_pending()
- [High] timer-econet: missing clockevent teardown on cpuhp_setup_state failure causes NULL dereference
--

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

clocksource/timer-econet-en751221: Do not use NR_CPUS for resource array size

Each pair of VPEs has one timer block, so the number of blocks is half
the number of CPUs on the hardware. This commit switches to an allocated
buffer for storing the iomem resources, as is done for the IRQs, avoiding
a too small array size when NR_CPUS is less than the number of hardware CPUs.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index edb3db3125ce3..9f28cc238372c 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
[ ... ]
> @@ -37,6 +36,10 @@ static DEFINE_PER_CPU(struct clock_event_device, econet_timer_pcpu);
>   */
>  static inline void __iomem *reg_ctl(u32 timer_n)
>  {
> +	/* reg_ctl() is used to check if a clock exists */
> +	if (timer_n >> 1 >= econet_timer.num_membase)
> +		return NULL;
> +

[Severity: High]
With reg_ctl() now returning NULL if the CPU index exceeds the mapped timer
blocks, what happens when it is called from cevt_is_pending()?

If the interrupt is a global IRQ, it can be routed to any CPU via userspace
smp_affinity, or fire as a spurious interrupt on an unsupported CPU. When
the IRQ handler cevt_interrupt() executes on a CPU without a backing timer
block, reg_ctl() will return NULL, and it looks like cevt_is_pending() will
pass this directly to ioread32():

return ioread32(reg_ctl(cpu_id)) & ctl_bit_pending(cpu_id);

Could this cause a kernel panic due to a NULL pointer dereference?

>  	return econet_timer.membase[timer_n >> 1];
>  }
>  
[ ... ]
> @@ -305,13 +312,17 @@ static int __init timer_init(struct device_node *np)
>  			irq_dispose_mapping(econet_timer.irqs[i]);
>  	}
>  out_membase:
> -	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
> +	for (int i = 0; i < num_blocks; i++) {
>  		if (econet_timer.membase[i]) {
>  			iounmap(econet_timer.membase[i]);
>  			econet_timer.membase[i] = NULL;
>  		}
>  	}
>  
> +	kfree(econet_timer.membase);
> +	econet_timer.membase = NULL;
> +

[Severity: High]
Does freeing the dynamically allocated membase array and setting it to NULL
here risk a NULL pointer dereference for already registered clockevents?

Looking at timer_init(), cpuhp_setup_state() is called without a teardown
callback. If cpuhp_setup_state() fails on a subset of CPUs, it aborts
without rolling back previously initialized CPUs, which leaves the clockevent
devices active.

When this error path executes and frees econet_timer.membase, the active
clockevent devices remain registered. When they fire, cevt_set_next_event()
calls reg_count(), which dereferences the now-NULL econet_timer.membase
pointer:

static inline void __iomem *reg_count(u32 timer_n)
{
	return econet_timer.membase[timer_n >> 1] + (timer_n & 1) * 0x08 + 0x08;
}

Could this lead to a kernel panic when the remaining active clockevents fire?

> +out_kfree_irqs:
>  	kfree(econet_timer.irqs);
>  	econet_timer.irqs = NULL;
>

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