Re: [PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing rollback in `lpi2c_imx_register_target()` when `pm_runtime_resume_and_get()` fails, leaving a dangling pointer in `lpi2c_imx->target`.
--

--- Patch [1]: [PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers ---
commit c2a5eb969b33b95968f240043a2bfa7ba6e85caf
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

This commit reads the VERID register during probe to check for target mode
support. It prevents accessing target registers on master-only controllers
and rejects target registration requests if unsupported.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
[ ... ]
> @@ -1330,6 +1335,10 @@ static int lpi2c_imx_register_target(struct i2c_client *client)
>  	struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
>  	int ret;
>  
> +	/* Reject target-mode registration on controllers that don't support it. */
> +	if (!lpi2c_imx->target_supported)
> +		return -EOPNOTSUPP;
> +
>  	if (lpi2c_imx->target)
>  		return -EBUSY;
>  
>  	lpi2c_imx->target = client;
>  
>  	ret = pm_runtime_resume_and_get(lpi2c_imx->adapter.dev.parent);
>  	if (ret < 0) {
>  		dev_err(&lpi2c_imx->adapter.dev, "failed to resume i2c controller");
>  		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling
pointer in the controller data?

If pm_runtime_resume_and_get() fails in lpi2c_imx_register_target(), the
function returns without clearing lpi2c_imx->target. Because the registration
failed, the target driver probe will fail and the i2c_client memory can be
freed (e.g., if dynamically removed or unbound).

Later, if the system is suspended and resumed, lpi2c_resume_noirq() will see
that lpi2c_imx->target is non-NULL and unconditionally dereference it:

drivers/i2c/busses/i2c-imx-lpi2c.c:lpi2c_resume_noirq() {
    ...
    if (lpi2c_imx->target)
        lpi2c_imx_target_init(lpi2c_imx);
    ...
}

Should lpi2c_imx->target be set back to NULL in this error path to avoid a
potential use-after-free crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803032705.2250373-1-carlos.song@oss.nxp.com?part=1