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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Accessing `se_clbk_hdl->dev_ctx->devname` outside `clbk_rx_lock` in the `cmd_tag` path leads to Use-After-Free or NULL pointer dereference.

Pre-existing issues:
- [High] Incorrect devm registration order causes `priv` to be freed while the RX mailbox channel is still active, leading to a NULL pointer dereference.
- [High] Memory leak of `priv` when early probe steps fail.
- [High] Unprotected read of `rx_msg_sz` creates a TOCTOU race that leads to response truncation on late interrupts.
--

--- Patch [4]: [PATCH v29 4/7] firmware: imx: device context dedicated to priv ---
commit de35bce80e0d23de288afde843dd6a5a471535dd
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.

Closes: https://sashiko.dev/#/patchset/20260717-imx-se-if-v28-0-0a9659c7e69d@nxp.com?part=4

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index e9405a82c54d9..a350e83a274a1 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
[ ... ]
> @@ -274,12 +283,13 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
>  		if (sz_mismatch)
>  			dev_err(dev,
> -				"CMD-RCVER NVM: hdr(0x%x) with different sz(%d != %d).\n",
> -				*(u32 *)header,
> +				"%s: CMD-RCVER NVM: hdr(0x%x) with different sz(%d != %d).\n",
> +				se_clbk_hdl->dev_ctx->devname, *(u32 *)header,

[Severity: High]
Is it possible for dev_ctx to be modified concurrently?

The patch correctly introduced a local devname variable to safely read under
clbk_rx_lock for the rsp_tag path below, but in this cmd_tag path,
se_clbk_hdl->dev_ctx->devname is accessed outside the lock.

Could this lead to a use-after-free or NULL pointer dereference if the command
receiver context is destroyed concurrently?

>  				(header->size << 2), rx_msg_sz);
>  	} else if (header->tag == priv->if_defs->rsp_tag) {
>  		bool exception_for_sz_mismatch = check_hdr_exception_for_sz(priv, header);
>  		u32 exp_rx_msg_sz = 0;
> +		const char *devname = NULL;
>  

[Severity: High]
This is a pre-existing issue, but does reading rx_msg_sz outside the lock
create a TOCTOU race?

If a late or spurious mailbox interrupt occurs from a previously timed-out
command, the IRQ handler could read a stale rx_msg_sz (reset to 0 during
cleanup):

	se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
	exp_rx_msg_sz = se_clbk_hdl->rx_msg_sz;
	spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);

After acquiring the lock, it might see the newly populated rx_msg from a new
sender, but copy 0 bytes and spuriously complete the new command. Should the
size be read under the lock?

>  		/*
>  		 * waiting_rsp_clbk_hdl.rx_msg is owned by the synchronous sender in
[ ... ]
> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 06068c4db8c1d..cd4d27dd98ce1 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> @@ -382,6 +418,12 @@ static int se_if_probe(struct platform_device *pdev)
>  		load_fw->is_fw_tobe_loaded = true;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but could the devm registration order lead to a
NULL pointer dereference?

In se_if_probe(), the mailbox channels are requested before the cleanup routine
is registered:

	ret = se_if_request_channel(dev, &priv->rx_chan, &priv->se_mb_cl, MBOX_RX_NAME);
	if (ret)
		return ret;

	ret = devm_add_action_or_reset(dev, se_if_probe_cleanup, pdev);

Because devres cleans up in LIFO order, se_if_probe_cleanup() will free priv
and clear drvdata while the RX mailbox channel is still active. If an interrupt
arrives in this window, se_if_rx_callback() will retrieve a NULL priv:

	priv = dev_get_drvdata(dev);
	...
	if (header->tag == priv->if_defs->cmd_tag) {

Should the devm action be registered before the mailbox channels are requested?

[Severity: High]
This is a pre-existing issue, but does this function leak the priv structure on
early error paths?

In se_if_probe(), priv is allocated manually using kzalloc_obj(). If the early
calls to se_if_request_channel() fail, the function returns early:

	ret = se_if_request_channel(dev, &priv->tx_chan, &priv->se_mb_cl, MBOX_TX_NAME);
	if (ret)
		return ret;

Since devm_add_action_or_reset() hasn't been called yet, the devres stack will
not execute the cleanup function, leaking priv. Should priv be freed explicitly
on these early error paths?

> +	ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed[0x%x] to create device contexts.",
> +				     ret);
> +
>  	/* By default, there is no pending FW to be loaded.*/

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-imx-se-if-v29-0-04a362f4fcca@nxp.com?part=4