Re: [PATCH] Input: apbps2: Simplify resource mapping and IRQ retrieval

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Requesting a shared IRQ before the serio port is fully allocated and initialized leads to a potential NULL pointer dereference or uninitialized spinlock access in the interrupt handler.
- [High] Mixing `devm` managed interrupts with manual resource unregistration causes an incorrect teardown order, leading to a use-after-free if a shared interrupt fires during device removal.
--

commit d306647eec008111ed14c378d300aa2cb98191b6
Author: Rosen Penev <rosenp@gmail.com>

Input: apbps2: Simplify resource mapping and IRQ retrieval

Simplify resource mapping by using devm_platform_ioremap_resource()
instead of the longer devm_platform_get_and_ioremap_resource() helper
as the last argument is NULL.

Additionally, use platform_get_irq() to retrieve the interrupt
instead of irq_of_parse_and_map() and propagate its error code on
failure. irq_of_parse_and_map() requires irq_dispose_mapping, which is
missing.

> diff --git a/drivers/input/serio/apbps2.c b/drivers/input/serio/apbps2.c
> index 0aa4ab00af352..5f21acdd41139 100644
> --- a/drivers/input/serio/apbps2.c
> +++ b/drivers/input/serio/apbps2.c
[ ... ]
> @@ -148,7 +148,10 @@ static int apbps2_of_probe(struct platform_device *ofdev)
>  	iowrite32be(0, &priv->regs->ctrl);
>  
>  	/* IRQ */
> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> +	irq = platform_get_irq(ofdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
>  	err = devm_request_irq(&ofdev->dev, irq, apbps2_isr,
>  				IRQF_SHARED, "apbps2", priv);

[Severity: High]
This is a pre-existing issue, but requesting a shared IRQ here before the
serio port is fully allocated and initialized creates a window for a NULL
pointer dereference.

If a shared interrupt fires immediately (triggered by another device on the
same line) and there is leftover data in the PS/2 FIFO, apbps2_isr() will
execute:

drivers/input/serio/apbps2.c:apbps2_isr() {
    ...
    serio_interrupt(priv->io, data, rxflags);
    ...
}

Since priv->io is either NULL or not fully initialized until later in probe:

drivers/input/serio/apbps2.c:apbps2_of_probe() {
    ...
    priv->io = ... /* allocated later */
    ...
}

Could this lead to a crash if the ISR fires early?

[Severity: High]
This is another pre-existing issue, but does mixing a devm-managed interrupt
here with manual unregistration in apbps2_of_remove() cause a teardown order
issue?

In apbps2_of_remove(), the driver manually unregisters and frees the serio
port:

drivers/input/serio/apbps2.c:apbps2_of_remove() {
    ...
    serio_unregister_port(priv->io);
    ...
}

Because the interrupt was requested with devm_request_irq(), its handler
remains active until devres runs free_irq() after apbps2_of_remove() returns.
If a shared interrupt fires during this window and there is pending data,
apbps2_isr() will execute and pass the already-freed priv->io pointer to
serio_interrupt(), resulting in a use-after-free.

Could the interrupt teardown order be adjusted to prevent this race?

>  	if (err) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603192415.6679-1-rosenp@gmail.com?part=1