RE: [PATCH] i2c: imx: Fix slave registration error path and missing NULL check

[Carlos Song (OSS)]

> -----Original Message-----
> From: Liem <liem16213@xxxxxxxxx>
> Sent: Thursday, June 25, 2026 3:12 PM
> To: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Frank Li <frank.li@xxxxxxx>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; Biwen Li
> <biwen.li@xxxxxxx>; Wolfram Sang <wsa@xxxxxxxxxx>;
> linux-i2c@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx; Liem <liem16213@xxxxxxxxx>
> Subject: [PATCH] i2c: imx: Fix slave registration error path and missing NULL check
> 
> [You don't often get email from liem16213@xxxxxxxxx. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> There are two issues that affect the i2c-imx slave handling:
> 
> 1. In i2c_imx_reg_slave(), i2c_imx->slave is checked at the beginning
>    and the function returns -EBUSY if it is non-NULL.  If
>    pm_runtime_resume_and_get() fails later, the error path returns
>    without clearing i2c_imx->slave, leaving it non-NULL.  Subsequent
>    attempts to register a slave will then immediately fail with
>    -EBUSY, making it impossible to register the slave again.  Fix
>    by setting i2c_imx->slave = NULL on the error path.
> 
> 2. In i2c_imx_unreg_slave(), the slave pointer is set to NULL after
>    disabling interrupts.  However, a pending interrupt might already
>    have started a timer (e.g. for slave event processing) before
>    the pointer was cleared.  The timer callback
>    i2c_imx_slave_event() dereferences i2c_imx->slave without a
>    NULL check, which results in a use-after-free / NULL pointer
>    dereference.  Prevent this by checking that i2c_imx->slave is
>    valid before calling i2c_slave_event() and updating the
>    last_slave_event field.
> 
> Both issues can trigger a kernel oops or permanent slave registration failure under
> certain race conditions.  Add the missing NULL assignment and the missing NULL
> check to harden the slave path.
> 
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Liem <liem16213@xxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-imx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> 28313d0fad37..4f7bcbeecfd0 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c


Hi, Liem 

Thank you very much for this fix!
Looks like you are catching a corner bug and try to fix this for i2c-imx target mode.

Have you meet the issue on one real platform?

> @@ -775,8 +775,10 @@ static void i2c_imx_enable_bus_idle(struct
> imx_i2c_struct *i2c_imx)  static void i2c_imx_slave_event(struct imx_i2c_struct
> *i2c_imx,
>                                 enum i2c_slave_event event, u8 *val)  {
> -       i2c_slave_event(i2c_imx->slave, event, val);
> -       i2c_imx->last_slave_event = event;
> +       if (i2c_imx->slave) {
> +               i2c_slave_event(i2c_imx->slave, event, val);
> +               i2c_imx->last_slave_event = event;
> +       }

>From i2c-imx.c driver, I notice this call trace is:

1. IRQ-> i2c_imx_slave_handle-> i2c_imx_slave_timeout(if in progress)-> i2c_imx_slave_finish_op
2. IRQ->i2c_imx_slave_finish_op

'''
In i2c_imx_unreg_slave(), the slave pointer is set to NULL after
disabling interrupts. However, a pending interrupt might already
have started a timer (e.g. for slave event processing) before
the pointer was cleared.
...

Yes, this may happen, then trigger slave hrtimer running. 

Go into i2c_imx_slave_finish_op().

you add a judgement in i2c_slave_event():

if (i2c_imx->slave) {
	i2c_slave_event(i2c_imx->slave, event, val);
	i2c_imx->last_slave_event = event;
}

At this time i2c_imx->slave= NULL, 
so i2c_slave_event(i2c_imx->slave, event, val) and i2c_imx->last_slave_event = event; won't run again.

in i2c_imx_slave_finish_op(),

Fall into" while (i2c_imx->last_slave_event != I2C_SLAVE_STOP)" the loop. So system maybe hang.

My idea is just cancel the slave timer and wait it finished after disabled IRQ.
If I am wrong please correct me. Thank you again.

static int i2c_imx_unreg_slave(struct i2c_client *client)
{
    imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);

    i2c_imx_reset_regs(i2c_imx);

+	hrtimer_cancel(&i2c_imx->slave_timer);

	i2c_imx->slave = NULL;
}

Carlos

> 
>  static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx) @@ -936,6
> +938,7 @@ static int i2c_imx_reg_slave(struct i2c_client *client)
>         /* Resume */
>         ret = pm_runtime_resume_and_get(i2c_imx->adapter.dev.parent);
>         if (ret < 0) {
> +               i2c_imx->slave = NULL;

This a good fix. I agree with this.

>                 dev_err(&i2c_imx->adapter.dev, "failed to resume i2c
> controller");
>                 return ret;
>         }
> --
> 2.34.1
>