Re: [PATCH v2 3/5] genirq: Export NMI APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Exporting `free_nmi()` and `disable_nmi()` allows loadable modules to manage NMIs, but these APIs fundamentally fail to synchronize with in-flight NMI handlers, introducing a race condition that leads to kernel panics on module unload.

Pre-existing issues:
- [High] `__cleanup_nmi()` fails to acquire `desc->request_mutex` when tearing down an NMI. This allows concurrent IRQ request/free operations to race, corrupting IRQ resource state and bypassing hardware resource allocation.
- [High] Concurrent NMI delivery during `free_nmi()` teardown leads to a NULL pointer dereference in `handle_fasteoi_nmi()` because `desc->action` is cleared before `__synchronize_hardirq()` waits for in-flight NMIs.
- [Medium] `free_nmi()` ignores the provided `dev_id` parameter, bypassing ownership validation and allowing any module to inadvertently or maliciously unregister an NMI owned by another subsystem.
--

commit cf44d4cc82f985321cdf77cbbbd3469b85de0426
Author: Mayank Rungta <mrungta@google.com>

genirq: Export NMI APIs

This commit exports request_nmi(), free_nmi(), enable_nmi(),
disable_nmi_nosync(), and disable_nmi() with EXPORT_SYMBOL_GPL to allow
loadable kernel modules to register and manage NMIs.

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 40013aa94481e..ded8cc33381cc 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c

[ ... ]

> @@ -2110,6 +2113,7 @@ const void *free_nmi(unsigned int irq, void *dev_id)
>  
>  	return __cleanup_nmi(irq, desc);
>  }
> +EXPORT_SYMBOL_GPL(free_nmi);

[Severity: High]
Is it safe to export free_nmi() and disable_nmi() given they can introduce
a race condition that leads to kernel panics on module unload?

When a module unloads, free_nmi() calls __cleanup_nmi(), which uses
__synchronize_hardirq() to wait for handlers on other CPUs to finish.
__synchronize_hardirq() checks the IRQD_IRQ_INPROGRESS flag:

kernel/irq/manage.c:__synchronize_hardirq() {
    ...
		while (irqd_irq_inprogress(&desc->irq_data))
			cpu_relax();
    ...
}

However, NMI handlers like handle_fasteoi_nmi() bypass standard IRQ locking
and never set IRQD_IRQ_INPROGRESS:

kernel/irq/chip.c:handle_fasteoi_nmi() {
    ...
	res = action->handler(irq, action->dev_id);
    ...
}

Consequently, __synchronize_hardirq() returns immediately. The module's text
segment could then be unmapped while the NMI handler is still executing,
leading to a page fault.

[Severity: High]
This is a pre-existing issue, but can concurrent NMI delivery during
free_nmi() teardown lead to a NULL pointer dereference in
handle_fasteoi_nmi()?

During free_nmi(), __cleanup_nmi() sets desc->action = NULL and drops
desc->lock before calling __synchronize_hardirq():

kernel/irq/manage.c:__cleanup_nmi() {
    ...
	desc->action = NULL;

	irq_settings_clr_disable_unlazy(desc);
	irq_shutdown(desc);
    ...
	/* Ensure all in-flight NMI handlers on other CPUs complete before freeing action */
	__synchronize_hardirq(desc, true);
    ...
}

If an NMI fires concurrently, the dispatcher handle_fasteoi_nmi() executes
on another CPU without taking desc->lock. It reads desc->action and
unconditionally dereferences it without checking for NULL:

kernel/irq/chip.c:handle_fasteoi_nmi() {
    ...
	struct irqaction *action = desc->action;
    ...
	res = action->handler(irq, action->dev_id);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but it appears __cleanup_nmi()
fails to acquire desc->request_mutex when tearing down an NMI. 

__cleanup_nmi() drops desc->lock and performs teardown (including
irq_release_resources()) without holding desc->request_mutex:

kernel/irq/manage.c:__cleanup_nmi() {
    ...
	irq_release_resources(desc);
    ...
}

Conversely, __setup_irq() relies exclusively on desc->request_mutex for
serialization. If __cleanup_nmi() runs concurrently with __setup_irq() (e.g.
during a rapid module reload or concurrent requests), they can race.
__setup_irq() may read a non-NULL desc->action and skip resource allocation:

kernel/irq/manage.c:__setup_irq() {
    ...
	if (!desc->action) {
		ret = irq_request_resources(desc);
    ...
}

Could this lead to bypassed hardware resource allocation or corrupted hardware
state for IRQ resources?

[Severity: Medium]
This is a pre-existing issue, but I noticed free_nmi() ignores the provided
dev_id parameter.

The free_nmi() API accepts a dev_id parameter, but delegates to
__cleanup_nmi() which drops the token entirely:

kernel/irq/manage.c:free_nmi() {
    ...
	return __cleanup_nmi(irq, desc);
}

Inside __cleanup_nmi(), the action is unconditionally unlinked and torn
down without checking if its dev_id matches the caller's cookie:

kernel/irq/manage.c:__cleanup_nmi() {
    ...
	if (!WARN_ON(desc->action == NULL)) {
		action = desc->action;
		irq_pm_remove_action(desc, action);
    ...
}

Unlike free_irq(), which verifies action->dev_id == dev_id, this lacks
ownership validation. Could a buggy loadable module unilaterally destroy
another component's NMI by passing an incorrect IRQ number or a
NULL cookie to free_nmi()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-qcom-wdt-nmi-series-v2-0-363979fe6b6b@google.com?part=3