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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Firmware can be tricked into reading out of bounds of the shared memory window, potentially accessing arbitrary physical memory.
- [High] A TOCTOU race condition allows multiple contexts to issue ELE_STORAGE_OPEN_REQ concurrently, breaking command receiver isolation and allowing one process to intercept another's NVM traffic.
- [High] Incorrect teardown ordering allows `fw_busy_work` to be scheduled after it is canceled, leading to a use-after-free when `priv` is freed.
- [High] A 3-way deadlock occurs during driver unbind if a concurrent `close()` is called on a context that recently experienced a command timeout.
- [Medium] The SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl unconditionally fails with -ENOMEM.
- [Medium] Memory and device reference leak on early probe error paths.
--

--- Patch [5]: [PATCH v41 5/7] firmware: imx: adds miscdev ---
commit 6943d8e80841760dc256faf0183efc19b3d26073
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
Subject: firmware: imx: adds miscdev

Adds the driver for communication interface to secure-enclave, that
enables exchanging messages with NXP secure enclave HW IP(s)
like EdgeLock Enclave, from User-Space Applications via character driver.

> 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
> @@ -66,0 +66,0 @@ int se_val_cmd_addrs(struct se_if_device_ctx *dev_ctx, struct se_api_msg *msg,
[ ... ]
> +		} else if (f->size_idx != SE_CMD_ADDR_NO_SIZE) {
[ ... ]
> +		} else if (f->buf_size) {
> +			/* buf_size: literal byte count (FW-defined constant). */
> +			if ((u64)f->buf_size > end - addr)
> +				return -EACCES;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Could a malicious userspace program trick the firmware into reading out of
bounds of the shared memory window?

The bounds check loop skips length verification entirely when size_idx is
SE_CMD_ADDR_NO_SIZE and buf_size is 0, only validating the start address.

For commands like ELE_OEM_AUTH_CONTAINER_REQ, this means userspace could
supply an address near the end of the authorized window, causing the enclave
firmware to read past the valid DMA window into unowned memory.

> @@ -618,0 +618,0 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
[ ... ]
> +		se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
> +		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
> +		if (!se_clbk_hdl->rx_msg) {
> +			if (atomic_read(&priv->fw_busy))
> +				schedule_fw_busy_work = true;
> +			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
> +
> +			if (schedule_fw_busy_work)
> +				schedule_work(&priv->fw_busy_work);

[Severity: High]
Is there a use-after-free risk if this callback runs during driver unbind?

If a late firmware response arrives while se_if_probe_cleanup() is running,
the callback might evaluate atomic_read(&priv->fw_busy) and prepare to
schedule the work. If cancel_work_sync() completes before schedule_work() is
called, the work will be queued after being canceled. When the unbind finishes
and frees priv, the workqueue will execute se_fw_busy_work() and dereference
the freed priv pointer.

> diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> --- a/drivers/firmware/imx/ele_fw_api.c
> +++ b/drivers/firmware/imx/ele_fw_api.c
> @@ -82,0 +82,0 @@ int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct se_msg_hdr
[ ... ]
> +	case ELE_STORAGE_OPEN_REQ:
> +		/* Might be cleared as part of tear down. */
> +		if (dev_ctx->strg_hdl) {
> +			ret = -EEXIST;
> +			break;
> +		}
> +		/*
[ ... ]
> +		 */
> +		scoped_guard(mutex, &priv->modify_lock)
> +			if (priv->cmd_receiver_clbk_hdl.dev_ctx &&
> +			    priv->cmd_receiver_clbk_hdl.dev_ctx != dev_ctx)
> +				ret = -EBUSY;

[Severity: High]
Does this create a time-of-check to time-of-use race condition that allows
multiple contexts to issue ELE_STORAGE_OPEN_REQ concurrently?

The exclusivity check is performed under a scoped modify_lock but dropped
before the command is actually sent. Two contexts could both pass this
validation because neither is registered yet.

> @@ -290,0 +290,0 @@ void fw_api_specific_ops(struct se_if_device_ctx *dev_ctx, struct se_api_msg *rx
[ ... ]
> +	case ELE_STORAGE_OPEN_REQ: {
> +		int rc;
> +
> +		/*
[ ... ]
> +		 */
> +		dev_ctx->strg_hdl = rx_msg->data[1];
> +
> +		if (is_cmd_interrupted) {
> +			if (se_close_storage(dev_ctx, dev_ctx->strg_hdl))
> +				dev_err(dev_ctx->priv->dev, "failed to close storage.\n");
> +			dev_ctx->strg_hdl = 0;
> +			break;
> +		}
> +
> +		rc = set_dev_ctx_as_command_receiver(dev_ctx, false);
> +		if (rc)
> +			dev_err(priv->dev,
> +				"Failed to register %s as CMD-Receiver: %d\n",
> +				dev_ctx->devname, rc);
> +		break;
> +	}

[Severity: High]
Could this result in broken isolation if two contexts obtain valid storage
handles?

Following the race mentioned above, the second process will fail to register
as a command receiver here. However, since this failure is only logged,
success is still returned to userspace. Could this cause the NVM callbacks
for the second process's storage handle to be routed to the first process?

> diff --git a/drivers/firmware/imx/ele_msg_addr_field.c b/drivers/firmware/imx/ele_msg_addr_field.c
> --- a/drivers/firmware/imx/ele_msg_addr_field.c
> +++ b/drivers/firmware/imx/ele_msg_addr_field.c
> @@ -20,0 +20,0 @@ static const struct se_cmd_addr_field ele_get_info_addr_fields[] = {
[ ... ]
> +static const struct se_cmd_addr_field ele_oem_auth_cntr_addr_fields[] = {
[ ... ]
> +	{ .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx = SE_CMD_ADDR_ALWAYS,
> +	  .size_idx = SE_CMD_ADDR_NO_SIZE },			/* container_hdr_addr */
> +};

(Just noting the definition for context of the bounds check issue.)

> 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
> @@ -702,0 +702,0 @@ static void se_if_probe_cleanup(void *plat_dev)
[ ... ]
> +	scoped_guard(mutex, &priv->se_if_cmd_lock) {
> +		cancel_work_sync(&priv->fw_busy_work);
> +		if (priv->rx_chan) {
> +			mbox_free_channel(priv->rx_chan);
> +			priv->rx_chan = NULL;
> +		}

[Severity: High]
Can a 3-way deadlock occur here during driver unbind if a concurrent close()
is called on a context that recently experienced a command timeout?

The unbind thread holds se_if_cmd_lock and waits for the workqueue via
cancel_work_sync().

The workqueue executing se_clear_fw_busy() holds nothing but waits for
dev_ctx->fops_lock.

The close thread executing cleanup_dev_ctx() holds fops_lock, observes
fw_busy == 0, and attempts to acquire se_if_cmd_lock by calling
se_close_storage().

This circular dependency seems like it could lock up the kernel unbind
process and userspace close().

[Severity: High]
Additionally, is the teardown ordering here safe?

By calling cancel_work_sync(&priv->fw_busy_work) before
mbox_free_channel(priv->rx_chan), the IRQ source remains active. This allows
the race condition mentioned earlier where the work could be re-armed after
being canceled.

> @@ -880,0 +880,0 @@ static int se_if_probe(struct platform_device *pdev)
[ ... ]
> +	priv = kzalloc_obj(*priv, GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	/*
[ ... ]
> +	 */
> +	get_device(priv->dev);
> +	kref_init(&priv->refcount);
> +	priv->if_defs = &if_node->if_defs;
> +	dev_set_drvdata(dev, priv);
> +
> +	ret = devm_mutex_init(dev, &priv->se_if_cmd_lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to init mutex: priv se_if_cmd_lock.\n");
> +	ret = devm_mutex_init(dev, &priv->modify_lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to init mutex: priv modify_lock.\n");
[ ... ]
> +	ret = devm_add_action_or_reset(dev, se_if_probe_cleanup, pdev);

[Severity: Medium]
Are there memory and device reference leaks on these early error paths?

If devm_mutex_init() fails, the function returns immediately. Because
devm_add_action_or_reset() has not been called yet, the cleanup function is
not registered, which leaves the priv allocation and the device reference
leaked.

> @@ -1012,0 +1012,0 @@ int set_dev_ctx_as_command_receiver(struct se_if_device_ctx *dev_ctx, bool is_io
[ ... ]
> +	if (!dev_ctx->strg_hdl)
> +		return -EINVAL;
> +
> +	if (is_ioctl)
> +		return -ENOMEM;

[Severity: Medium]
Does this make the SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl completely unusable?

Because se_ioctl() invokes this function with is_ioctl set to true, this
check will cause the ioctl to fail unconditionally with -ENOMEM.

> @@ -1844,0 +1844,0 @@ static long se_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
[ ... ]
> +		case SE_IOCTL_ENABLE_CMD_RCV_STATUS: {
> +			err = set_dev_ctx_as_command_receiver(dev_ctx, true);
> +			if (err)
> +				dev_err(priv->dev, "Failed[%ld] to register %s as CMD-Receiver.\n",
> +					err, dev_ctx->devname);
> +		break;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-imx-se-if-v41-0-29cf129e4d86@nxp.com?part=5