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

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

[Liem]

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@vger.kernel.org
Signed-off-by: Liem <liem16213@gmail.com>
---
 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
@@ -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;
+	}
 }
 
 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;
 		dev_err(&i2c_imx->adapter.dev, "failed to resume i2c controller");
 		return ret;
 	}
-- 
2.34.1

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

[Carlos Song (OSS)]

> -----Original Message-----
> From: Liem <liem16213@gmail.com>
> Sent: Thursday, June 25, 2026 3:12 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: Andi Shyti <andi.shyti@kernel.org>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Frank Li <frank.li@nxp.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; Biwen Li
> <biwen.li@nxp.com>; Wolfram Sang <wsa@kernel.org>;
> linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org; Liem <liem16213@gmail.com>
> Subject: [PATCH] i2c: imx: Fix slave registration error path and missing NULL check
> 
> [You don't often get email from liem16213@gmail.com. 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@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>
> ---
>  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
> 

[PATCH v2] i2c: imx: Fix slave registration error path and missing timer cleanup

[Liem]

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 the hrtimer (i2c_imx_slave_timeout) before the pointer
   was cleared.  If the hrtimer fires after i2c_imx->slave is set to
   NULL, the timer callback i2c_imx_slave_finish_op() will call
   i2c_imx_slave_event() with a NULL slave pointer, and the
   last_slave_event check loop in i2c_imx_slave_finish_op() may cause
   a system hang because last_slave_event is no longer updated.  Fix
   by canceling the hrtimer and waiting for it to complete after
   disabling interrupts, before clearing the slave pointer.

Both issues can trigger a kernel oops, system hang, or permanent
slave registration failure under certain race conditions.  Add the
missing NULL assignment and the missing hrtimer cleanup to harden
the slave path.

Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
Cc: stable@vger.kernel.org
Signed-off-by: Liem <liem16213@gmail.com>
---
v1 -> v2:
  - Instead of adding a NULL check in i2c_imx_slave_event(), cancel
    the hrtimer and wait for it to finish in i2c_imx_unreg_slave()
    after disabling interrupts, as suggested by <Carlos Song>.
    This avoids a potential hang in the last_slave_event loop in
    i2c_imx_slave_finish_op().
---
 drivers/i2c/busses/i2c-imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 28313d0fad37..04ffb927aba9 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -936,6 +936,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;
 		dev_err(&i2c_imx->adapter.dev, "failed to resume i2c controller");
 		return ret;
 	}
@@ -957,7 +958,7 @@ 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;
 
 	/* Suspend */
-- 
2.53.0

Re: [PATCH v2] i2c: imx: Fix slave registration error path and missing timer cleanup

[Frank Li]

On Fri, Jun 26, 2026 at 12:02:19AM +0800, Liem wrote:
>
> 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 the hrtimer (i2c_imx_slave_timeout) before the pointer
>    was cleared.  If the hrtimer fires after i2c_imx->slave is set to
>    NULL, the timer callback i2c_imx_slave_finish_op() will call
>    i2c_imx_slave_event() with a NULL slave pointer, and the
>    last_slave_event check loop in i2c_imx_slave_finish_op() may cause
>    a system hang because last_slave_event is no longer updated.  Fix
>    by canceling the hrtimer and waiting for it to complete after
>    disabling interrupts, before clearing the slave pointer.

Please use two patches to fix these problem. One patch fix one problem.

Frank

>
> Both issues can trigger a kernel oops, system hang, or permanent
> slave registration failure under certain race conditions.  Add the
> missing NULL assignment and the missing hrtimer cleanup to harden
> the slave path.
>
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>
> ---
> v1 -> v2:
>   - Instead of adding a NULL check in i2c_imx_slave_event(), cancel
>     the hrtimer and wait for it to finish in i2c_imx_unreg_slave()
>     after disabling interrupts, as suggested by <Carlos Song>.
>     This avoids a potential hang in the last_slave_event loop in
>     i2c_imx_slave_finish_op().
> ---
>  drivers/i2c/busses/i2c-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37..04ffb927aba9 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -936,6 +936,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;
>                 dev_err(&i2c_imx->adapter.dev, "failed to resume i2c controller");
>                 return ret;
>         }
> @@ -957,7 +958,7 @@ 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;
>
>         /* Suspend */
> --
> 2.53.0
>
>