Re: [PATCH v27 4/7] firmware: imx: device context dedicated to priv

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `init_misc_device_context()` unconditionally returns `-ENOMEM` on the success path, causing device probe to always fail.
- [High] Concurrent execution and lock dropping between `ele_msg_send_rcv` and `se_if_rx_callback` causes a NULL pointer dereference on `dev_ctx`.
--

--- Patch [4]: [PATCH v27 4/7] firmware: imx: device context dedicated to priv ---
commit 161a8ae1e5c289f19271beb8c2724ea2a799027b
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: device context dedicated to priv

Add priv_dev_ctx to prepare enabling misc-device context based send-receive
path, to communicate with FW. No functionality change.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 3f5f78df49b52..955b7409ff9fc 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -167,11 +171,13 @@ int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
>  		if (err > 0)
>  			err = -ERESTARTSYS;
>  		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> -		dev_err(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
> +		dev_err(priv->dev, "%s: Err[0x%x]:Interrupted by signal.",
> +			dev_ctx->devname, err);
>  	}
>  
>  clear_waiter:
>  	spin_lock_irqsave(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
> +	priv->waiting_rsp_clbk_hdl.dev_ctx = NULL;
>  	priv->waiting_rsp_clbk_hdl.rx_msg = NULL;
>  	priv->waiting_rsp_clbk_hdl.rx_msg_sz = 0;
>  	spin_unlock_irqrestore(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);

[ ... ]

> @@ -305,8 +311,8 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  
>  		if (sz_mismatch)
>  			dev_err(dev,
> -				"Rsp to CMD: hdr(0x%x) with different sz(%d != %d).\n",
> -				*(u32 *)header,
> +				"%s: Rsp to CMD: hdr(0x%x) with different sz(%d != %d).\n",
> +				se_clbk_hdl->dev_ctx->devname, *(u32 *)header,

[Severity: High]
Can this result in a NULL pointer dereference if the awakened thread clears
the context?

In se_if_rx_callback(), complete() is called and the lock is dropped right
before checking sz_mismatch:

    complete(&se_clbk_hdl->done);
    spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);

This wakes up ele_msg_send_rcv() which proceeds to the clear_waiter
cleanup path:

    priv->waiting_rsp_clbk_hdl.dev_ctx = NULL;

If the awakened thread clears dev_ctx before the dev_err() is executed,
will accessing se_clbk_hdl->dev_ctx->devname here crash the kernel?

>  				(header->size << 2), exp_rx_msg_sz);
>  	} else {
>  		dev_err(dev, "Failed to select a device for message: %.8x\n",

[ ... ]

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 7cb98a373334b..25f9cd5441f90 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> @@ -197,6 +197,36 @@ static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se
>  	return 0;
>  }
>  
> +static int init_misc_device_context(struct se_if_priv *priv, int ch_id,
> +				    struct se_if_device_ctx **new_dev_ctx)
> +{
> +	const char *err_str = "Failed to allocate memory";
> +	struct se_if_device_ctx *dev_ctx;
> +	int ret = -ENOMEM;
> +
> +	dev_ctx = kzalloc_obj(*dev_ctx, GFP_KERNEL);
> +
> +	if (!dev_ctx)
> +		return ret;
> +
> +	dev_ctx->devname = kasprintf(GFP_KERNEL, "%s0_ch%d",
> +				     get_se_if_name(priv->if_defs->se_if_type),
> +				     ch_id);
> +	if (!dev_ctx->devname)
> +		goto exit;
> +
> +	dev_ctx->priv = priv;
> +	*new_dev_ctx = dev_ctx;
> +
> +	return ret;

[Severity: High]
Does this code always fail and cause the probe to abort?

The ret variable is initialized to -ENOMEM at the top of the function but is
never set to 0 on the success path. When se_if_probe() calls this function,
will it always receive an error and abort the driver initialization?

> +exit:
> +	*new_dev_ctx = NULL;
> +
> +	kfree(dev_ctx->devname);
> +	kfree(dev_ctx);
> +	return dev_err_probe(priv->dev, ret, "%s", err_str);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-imx-se-if-v27-0-bb7c45952f06@nxp.com?part=4