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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] se_if_probe_cleanup() frees priv->priv_dev_ctx before freeing the mailbox channels, creating a race condition where the mailbox RX callback can access a freed devname string on the error logging path.
--

--- Patch [4]: [PATCH v31 4/7] firmware: imx: device context dedicated to priv ---
commit 29854db2c90371c5e52ff11225c81835904c4be3
Author: Pankaj Gupta <[email protected]>

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.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=4

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -294,13 +301,14 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  		 * Clamp the copy length to the pre-allocated receiver buffer (MAX_NVM_MSG_LEN).
>  		 */
>  		se_clbk_hdl->rx_msg_sz = min_t(u32, rx_msg_sz, MAX_NVM_MSG_LEN);
> +		devname = se_clbk_hdl->dev_ctx->devname;
>  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
>  		complete(&se_clbk_hdl->done);
>  		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",
> +				devname, *(u32 *)header,
>  				(header->size << 2), rx_msg_sz);

[ ... ]

> @@ -335,14 +343,15 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  			sz_mismatch = true;
>  
>  		se_clbk_hdl->rx_msg_sz = min(rx_msg_sz, exp_rx_msg_sz);
> +		devname = se_clbk_hdl->dev_ctx->devname;
>  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
>  		complete(&se_clbk_hdl->done);
>  		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
>  
>  		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",
> +				devname, *(u32 *)header,
>  				(header->size << 2), exp_rx_msg_sz);

[ ... ]

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> @@ -309,6 +339,11 @@ static void se_if_probe_cleanup(void *plat_dev)
>  	if (!priv)
>  		return;
>  
> +	if (priv->priv_dev_ctx) {
> +		kfree(priv->priv_dev_ctx->devname);
> +		kfree(priv->priv_dev_ctx);
> +	}
> +
>  	if (priv->rx_chan)
>  		mbox_free_channel(priv->rx_chan);
>  	if (priv->tx_chan)

[Severity: High]
Can this sequence result in a use-after-free of devname?

If the firmware sends a response with an unexpected size during a probe
transaction, sz_mismatch becomes true. In se_if_rx_callback(), the code
caches devname, calls complete(), drops the lock, and then calls dev_err()
passing the cached devname:

drivers/firmware/imx/ele_common.c:se_if_rx_callback()
        devname = se_clbk_hdl->dev_ctx->devname;
        ...
        complete(&se_clbk_hdl->done);
        spin_unlock_irqrestore(...);
        
        if (sz_mismatch)
                dev_err(dev, "%s ...", devname, ...);
                
Calling complete() wakes up the waiting probe thread. If the probe aborts
due to this mismatch and triggers devres cleanup, se_if_probe_cleanup()
runs.

Because se_if_probe_cleanup() frees priv_dev_ctx and its devname before
calling mbox_free_channel() (which would otherwise synchronize and stop
pending callbacks), devname could be freed while the callback thread is
preempted before or during the dev_err() call.

Would it be safer to free priv_dev_ctx after the mailbox channels are freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4