Re: [PATCH v2 06/11] media: sunxi-cir: Ensure no more interrupts can occur before free

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

Pre-existing issues:
- [High] The teardown sequence fails to synchronize with or disable the active devm-managed IRQ handler, leaving a race window that guarantees either a fatal bus error (unclocked register access) or a Use-After-Free.
- [High] Device interfaces and the IRQ handler are registered before hardware initialization in probe(), allowing userspace or interrupts to access unclocked registers and cause a bus fault.
- [High] The suspend callback disables clocks without synchronizing or disabling the device interrupt, allowing an unclocked MMIO access race.
--

--- Patch [6]: [PATCH v2 06/11] media: sunxi-cir: Ensure no more interrupts can occur before free ---
commit 2912f57e54d0f869a5daead9e5ddf88c0db3d980
Author: Sean Young <[email protected]>
media: sunxi-cir: Ensure no more interrupts can occur before free

This commit attempts to fix a race condition during module removal by moving
the call to rc_free_device() after sunxi_ir_hw_exit().

> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -374,8 +374,8 @@ static void sunxi_ir_remove(struct platform_device *pdev)
>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
>  
>  	rc_unregister_device(ir->rc);
> -	rc_free_device(ir->rc);
>  	sunxi_ir_hw_exit(&pdev->dev);
> +	rc_free_device(ir->rc);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
leave a race window with the devm-managed IRQ handler?

The commit message mentions ensuring no more interrupts can occur before
free, but sunxi_ir_hw_exit() only disables the clocks and asserts reset:

sunxi_ir_hw_exit() {
        clk_disable_unprepare(ir->clk);
        clk_disable_unprepare(ir->apb_clk);
        reset_control_assert(ir->rst);
}

Because the IRQ is managed by devm_request_irq(), it remains active until
after sunxi_ir_remove() returns. If an interrupt fires concurrently after
the clocks are disabled, sunxi_ir_irq() will attempt to access unclocked
MMIO registers:

sunxi_ir_irq() {
        ...
        status = readl(ir->base + SUNXI_IR_RXSTA_REG);

Could this lead to a synchronous external abort? Should there be an
explicit disable_irq() or similar synchronization before disabling the
clocks?

Additionally, this same pattern appears to exist in sunxi_ir_suspend(),
which also calls sunxi_ir_hw_exit() while the IRQ is still globally active.

[Severity: High]
This is also a pre-existing issue, but is there a similar unclocked access
risk during driver initialization?

In sunxi_ir_probe(), rc_register_device() and devm_request_irq() are called
before the hardware clocks are enabled via sunxi_ir_hw_init():

sunxi_ir_probe() {
        ...
        ret = rc_register_device(ir->rc);
        ...
        ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
        ...
        ret = sunxi_ir_hw_init(dev);
}

If userspace immediately issues a LIRC_SET_REC_TIMEOUT ioctl after the
device is registered, sunxi_ir_set_timeout() will write to SUNXI_IR_CIR_REG
before the clocks are enabled. Similarly, a pending interrupt could cause
sunxi_ir_irq() to run before sunxi_ir_hw_init().

Should the hardware initialization be moved before exposing the device and
requesting the IRQ?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6