Re: [PATCH v4 2/2] i2c: imx: Cancel hrtimer before clearing slave pointer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fails to completely resolve the NULL pointer dereference and timer re-arming race because it does not synchronize with the hardware interrupt handler (`i2c_imx_isr`).
- [High] Unsafe register access in shared interrupt handler while device is runtime suspended.
--

commit c2a34f9b292f8911be5d2996abfed1cdbdad49e7
Author: Liem <liem16213@gmail.com>

i2c: imx: Cancel hrtimer before clearing slave pointer

This commit attempts to fix a use-after-free and NULL pointer dereference
by canceling the slave hrtimer before setting the slave pointer to NULL in
i2c_imx_unreg_slave(). The goal is to prevent the timer callback from firing
and dereferencing a cleared pointer.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 136e179d18b2d..4a6348f03fc5d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -895,6 +895,7 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
>  
>  	i2c_imx_reset_regs(i2c_imx);
>  
> +	hrtimer_cancel(&i2c_imx->slave_timer);

[Severity: High]
This is a pre-existing issue, but does this patch fully close the race
condition it attempts to fix?

Since teardown happens without holding slave_lock or calling
synchronize_irq(), the hardware interrupt handler (i2c_imx_isr) can
execute concurrently.

If the ISR evaluates i2c_imx->slave before it is set to NULL here, it will
proceed to i2c_imx_slave_handle() and unconditionally re-arm the timer:

drivers/i2c/busses/i2c-imx.c:i2c_imx_slave_handle() {
    ...
    hrtimer_restart(&i2c_imx->slave_timer);
    ...
}

Because i2c_imx_isr() is a hardware interrupt, hrtimer_cancel() here will
return immediately without waiting for it. The concurrent ISR will re-arm
the timer, which can then fire after i2c_imx->slave is set to NULL, still
causing the NULL pointer dereference.

Should synchronize_irq() be called before clearing the pointer and
canceling the timer, or should the slave_lock be held?

>  	i2c_imx->slave = NULL;
>  
>  	/* Suspend */

[Severity: High]
This is a pre-existing issue, but can the shared interrupt handler crash the
system while the device is suspended?

The interrupt is requested as IRQF_SHARED in i2c_imx_probe():

    ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED,
                               pdev->name, i2c_imx);

When the device is suspended, i2c_imx_runtime_suspend() disables the clock
via clk_disable(i2c_imx->clk) without masking the shared IRQ line.

If another device on the same IRQ line triggers an interrupt while the
controller is suspended, i2c_imx_isr() will execute and unconditionally
read hardware registers:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr() {
    ...
    status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
    ...
}

On i.MX hardware, accessing registers of an unclocked peripheral results
in a synchronous bus fault. Does the ISR need a check to ensure the device
is not runtime-suspended before reading registers?

[ ... ]

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