Re: [PATCH v31 5/7] firmware: imx: adds miscdev

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

New issues:
- [Critical] Kernel panic due to traversing an uninitialized list head (`mem_pool_buf_list`) during device context cleanup.
- [Critical] The driver allows user-space to construct raw command payloads and embed unvalidated DMA addresses, leading to a Confused Deputy vulnerability.
- [High] Firmware commands sent to the command receiver can overwrite unprocessed commands, leading to lost events and spurious `-ENODEV` failures.
- [High] The SE_IOCTL_CMD_SEND_RCV_RSP ioctl incorrectly returns a positive integer to userspace when the firmware command fails or the response validation fails, masking the failure and skipping subsequent cleanup error checks.
- [High] A TOCTOU race between `ele_msg_send_rcv()` checking the `going_away` flag and `se_if_probe_cleanup()` forcing a wakeup causes a lost wakeup during unbind, leading to a permanent deadlock.
- [High] A slab-out-of-bounds read occurs in `se_val_rsp_hdr_n_status` when handling specific debug commands with a small `rx_buf_sz`.

Pre-existing issues:
- [High] Internal kernel hardware operations (like IMEM save/restore during suspend/resume) fail spuriously if a signal is pending, breaking PM transitions.
--

--- Patch [5]: [PATCH v31 5/7] firmware: imx: adds miscdev ---
commit 81281c9d3b1f75f8ca39e2d84326a1eb70748f81
Author: Pankaj Gupta <[email protected]>

firmware: imx: adds miscdev
    
This commit adds the character driver to enable communication with the NXP
secure enclave hardware from user-space applications. It introduces ioctls
for shared memory management and exchanging messages with the firmware.
Several checks are implemented on incoming message headers to block
restricted commands.

> 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
> @@ -246,6 +246,14 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
>  	guard(mutex)(&priv->se_if_cmd_lock);
>  
> +	/*
> +	 * Teardown has begun: do not arm a new transaction. A thread that was
> +	 * blocked on se_if_cmd_lock while se_if_probe_cleanup() ran its
> +	 * complete_all() would otherwise miss that wake-up, re-arm the
> +	 * completion below and wait the full timeout, while unbind blocks
> +	 * indefinitely on this thread's fops_lock. Bail out instead.
> +	 */
> +	if (atomic_read(&priv->going_away))
> +		return -ENODEV;
> +
>  	if (atomic_read(&priv->fw_busy)) {
>  		dev_dbg(priv->dev, "%s: ELE became unresponsive.\n", dev_ctx->devname);
>  		return -EBUSY;
>  	}
> +
>  	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);

[Severity: High]
Is there a TOCTOU race condition here with se_if_probe_cleanup()? If
se_if_probe_cleanup() sets going_away = 1 and calls complete_all() without
acquiring se_if_cmd_lock, could it execute complete_all() right after this
atomic_read() check but before reinit_completion() is called? Would this
erase the forced teardown wakeup and cause a permanent deadlock?

[ ... ]

> @@ -279,6 +284,14 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
>  	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
>  		/*
>  		 * Signal was deferred until the FW/kernel protocol resynchronized.
>  		 * On success report -ERESTARTSYS for the interrupted wait; the
>  		 * command is not re-sent. Keep real errors like -ETIMEDOUT.
>  		 */
>  		if (err > 0)
>  			err = -ERESTARTSYS;
>  		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;

[Severity: High]
This isn't a bug introduced by this patch, but does this explicit override
of a successful hardware interaction with -ERESTARTSYS break Power Management
transitions? If a signal is pending during system suspend, could this cause
internal kernel hardware operations like se_save_imem_state() to fail
spuriously and permanently lose the enclave IMEM state required for proper
resume?

[ ... ]

> @@ -323,9 +395,13 @@ 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);

[Severity: High]
Does this unconditionally overwrite the shared rx_msg buffer and call
complete() upon receiving a firmware command? If the firmware issues a
second command before the userspace daemon executes a read() to consume the
first one, could the first command's payload be completely overwritten,
resulting in a spurious -ENODEV failure?

> 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
> @@ -284,11 +292,308 @@ static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se
> +static int init_se_shared_mem(struct se_if_device_ctx *dev_ctx)
> +{
> +	struct se_shared_mem_mgmt_info *se_shared_mem_mgmt = &dev_ctx->se_shared_mem_mgmt;
> +	struct se_if_priv *priv = dev_ctx->priv;
> +
> +	INIT_LIST_HEAD(&se_shared_mem_mgmt->pending_out);
> +	INIT_LIST_HEAD(&se_shared_mem_mgmt->pending_in);
> +
> +	if (priv->mem_pool)
> +		INIT_LIST_HEAD(&se_shared_mem_mgmt->mem_pool_buf_list);

[Severity: Critical]
If priv->mem_pool is NULL, the mem_pool_buf_list remains zero-initialized.
Will se_cleanup_mem_pool_buf() later unconditionally traverse this
uninitialized list head via list_for_each_entry_safe() and cause a kernel
panic when the file descriptor is closed?

[ ... ]

> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	struct se_api_msg *tx_msg __free(kfree) =
> +		memdup_user(u64_to_user_ptr(cmd_snd_rcv_rsp_info.tx_buf),
> +			    cmd_snd_rcv_rsp_info.tx_buf_sz);
> +	if (IS_ERR(tx_msg)) {
> +		err = PTR_ERR(tx_msg);
> +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +		return err;
> +	}

[Severity: Critical]
Does this allow user-space to construct raw command payloads and embed
unvalidated DMA addresses? Since these addresses are not validated to ensure
they point to memory allocated via SE_IOCTL_SETUP_IOBUF before calling
ele_msg_send(), could a malicious application embed spoofed physical
addresses (like kernel page tables) and instruct the hardware to access
them?

[ ... ]

> +	struct se_api_msg *rx_msg __free(kfree) =
> +		kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL);
> +	if (!rx_msg) {
> +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +		return -ENOMEM;
> +	}

[Severity: High]
Can this lead to a slab-out-of-bounds read in se_val_rsp_hdr_n_status()?
If rx_buf_sz is small (e.g., 4 bytes) but the firmware returns a header size
larger than SE_MU_HDR_WORD_SZ for debug commands, will
check_hdr_exception_for_sz() bypass the mismatch check and cause
se_val_rsp_hdr_n_status() to read past the 4-byte allocated rx_msg buffer?

[ ... ]

> +	rsp_status_err =
> +		se_val_rsp_hdr_n_status(priv, rx_msg, tx_msg->header.command,
> +					cmd_snd_rcv_rsp_info.rx_buf_sz,
> +					tx_msg->header.ver == priv->if_defs->base_api_ver);
> +
> +	if (!rsp_status_err) {
> +		err = se_dev_ctx_cpy_out_data(dev_ctx);
> +		if (err < 0) {
> +			se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +			return err;
> +		}
> +	}

[Severity: High]
Does this mask the failure when the firmware command fails? If
se_val_rsp_hdr_n_status() sets a negative error code in rsp_status_err,
it skips se_dev_ctx_cpy_out_data(), but the positive message size from
ele_msg_send_rcv() stored in err isn't updated. Could this cause the ioctl
to return a positive value to userspace, falsely indicating success?

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