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

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

New issues:
- [Medium] The `devname` string pointer is cached inside `clbk_rx_lock` but dereferenced after the lock is released and the completion is signaled, leading to a potential Use-After-Free.

Pre-existing issues:
- [High] Returning `-ERESTARTSYS` after a successful firmware transaction causes the VFS to silently retry the ioctl, leading to duplicate execution of non-idempotent firmware commands.
--

--- Patch [4]: [PATCH v35 4/7] firmware: imx: device context dedicated to priv ---
commit df99e6bab818347c619050f003cdaa34c153bd90
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. No functionality change.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index bc08ea2dcd87d..707fb69431baa 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -214,7 +219,8 @@ int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
>  		if (err > 0)
>  			err = -ERESTARTSYS;

[Severity: High]
This is a pre-existing issue, but does returning -ERESTARTSYS after a
successful firmware transaction cause problems with the VFS layer?

If a signal is delivered to the process while waiting, ele_msg_rcv() catches
it but waits uninterruptibly because the command is already in flight. Once
the firmware successfully responds, ele_msg_rcv() returns the positive
response size.

However, in ele_msg_send_rcv(), seeing signal_rcvd is true causes the
positive return value to be explicitly replaced with -ERESTARTSYS:

		if (err > 0)
			err = -ERESTARTSYS;

Would this cause the VFS layer to automatically restart the syscall,
leading to the same firmware command being issued a second time, which
might be unsafe for non-idempotent operations like key generation?

>  		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> -		dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
> +		dev_dbg(priv->dev, "%s: Err[0x%x]:Interrupted by signal.",
> +			dev_ctx->devname, err);
>  	}

[ ... ]

> @@ -296,13 +303,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);

[Severity: Medium]
Could this code trigger a use-after-free of the devname string when dynamic
file descriptor lifetimes are fully implemented?

In se_if_rx_callback(), the devname pointer is cached locally, but the lock
is then dropped and complete() is called. This can wake up a waiting thread.

If the waking thread immediately closes the file descriptor, could dev_ctx
and its dynamically allocated devname string be freed before the dev_err()
statement above finishes executing?

[ ... ]

> @@ -337,14 +345,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);

[Severity: Medium]
Does this branch also share the same potential use-after-free pattern as the
cmd_tag handling branch above, if the waking thread frees dev_ctx before
dev_err() completes?

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