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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Arbitrary physical memory read/write vulnerability (Confused Deputy) via SE_IOCTL_CMD_SEND_RCV_RSP.
- [High] Use-After-Free of `gate` and `miscdev` due to race between unbind and VFS `open`.
- [High] Unbind deadlock/hang for up to 5000 seconds due to incorrect ordering of `complete_all` and `fops_lock` acquisition.
- [High] Overwriting a successful firmware transaction with `-ERESTARTSYS` leads to double execution with zeroed DMA buffers.
- [High] NULL pointer dereference in `se_if_rx_callback` due to clearing `drvdata` while the mailbox channel is active.
- [High] Out-of-bounds read in `se_if_rx_callback` by trusting unvalidated firmware payload length.
- [High] Premature freeing of `gen_pool` DMA buffer on timeout risks silent memory corruption.
- [Medium] Lockdep false positive 'possible recursive locking detected' on `fops_lock`.
- [Medium] Early device exposure to userspace before probe initialization is complete.
- [Low] Implicit compiler padding in UAPI structure `se_ioctl_cmd_snd_rcv_rsp_info`.
--

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

This patch adds the driver for a communication interface to the NXP
secure-enclave. It exposes a character device to userspace for exchanging
messages with hardware IPs like EdgeLock Enclave. It also implements
command validation, shared memory management, and power/reset command
restrictions.

> diff --git a/include/uapi/linux/se_ioctl.h b/include/uapi/linux/se_ioctl.h
> new file mode 100644
> index 0000000000000..ea14cec05020d
> --- /dev/null
> +++ b/include/uapi/linux/se_ioctl.h
> @@ -0,0 +1,97 @@
[ ... ]
> +struct se_ioctl_cmd_snd_rcv_rsp_info {
> +	__u64 tx_buf;
> +	__u32 tx_buf_sz;
> +	__u64 rx_buf;
> +	__u32 rx_buf_sz;
> +};

[Severity: Low]
Will this struct have implicit compiler padding after the 32-bit fields,
potentially causing UAPI size differences across architectures?

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 524ebc0f697e6..28b835f2880fb 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -15,24 +15,65 @@
[ ... ]
> +static void ele_get_info_cleanup(struct se_if_priv *priv, u32 *buf, dma_addr_t d_addr,
> +				 size_t size)
> +{
[ ... ]
> +	if (priv->mem_pool)
> +		gen_pool_free(priv->mem_pool, (unsigned long)buf, size);
> +	else if (!se_is_fw_busy_ctx(priv->priv_dev_ctx))
> +		se_dev_ctx_shared_mem_cleanup(priv->priv_dev_ctx);
> +}

[Severity: High]
Could the gen_pool_free() path free memory while the hardware is still busy?
The standard allocation path uses a se_is_fw_busy_ctx() guard to prevent
freeing memory the hardware might still write to, but that guard is missing
here, risking silent memory corruption if a timeout occurred.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index a350e83a274a1..fbb6e584341a1 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -226,6 +281,7 @@ static bool check_hdr_exception_for_sz(struct se_if_priv *priv,
>  void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  {
>  	struct se_clbk_handle *se_clbk_hdl;
> +	bool schedule_fw_busy_work = false;
>  	struct device *dev = mbox_cl->dev;
>  	struct se_msg_hdr *header;
>  	bool sz_mismatch = false;
> @@ -305,9 +361,13 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
[ ... ]
>  	priv = dev_get_drvdata(dev);
>  
>  	/* The function can be called with NULL msg */
>  	if (IS_ERR_OR_NULL(msg)) {
[ ... ]
>  	header = msg;
>  	rx_msg_sz = header->size << 2;
>  
>  	/* Incoming command: wake up the receiver if any. */
>  	if (header->tag == priv->if_defs->cmd_tag) {

[Severity: High]
Can priv be NULL here if the callback fires during driver unbind?
In se_if_probe_cleanup(), dev_set_drvdata() clears the private data pointer
before devres frees the active mailbox channels. A late interrupt would
dereference the NULL pointer.

> [ ... ]
>  		se_clbk_hdl->rx_msg_sz = rx_msg_sz;
>  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
>  		complete(&se_clbk_hdl->done);

[Severity: High]
Does this blindly trust the firmware-provided header->size for the copy?
If the mailbox driver's msg buffer is smaller than rx_msg_sz, this could
result in an out-of-bounds read that copies arbitrary kernel memory into
the response buffer returned to userspace.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index cd4d27dd98ce1..3c14964830a6d 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
> @@ -243,11 +250,269 @@ static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se
[ ... ]
> +static int load_firmware(struct se_if_priv *priv, const u8 *se_img_file_to_load)
> +{
[ ... ]
> +	/*
> +	 * Serialize access to priv_dev_ctx shared memory to prevent pos
> +	 * corruption if two driver-internal callers run concurrently (e.g.
> +	 * ele_get_info() racing with load_firmware()).
> +	 */
> +	scoped_guard(mutex, &priv->priv_dev_ctx->fops_lock) {
> +		se_fw_buf_len = fw->size;

[Severity: Medium]
Could this trigger a lockdep warning for recursive locking? The ioctl path
already holds a fops_lock, and taking the internal context's lock of the
same class here is valid hierarchical locking but might be flagged as a
deadlock by static analysis.

> @@ -261,18 +526,909 @@ static int init_misc_device_context(struct se_if_priv *priv, int ch_id,
[ ... ]
> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	err = ele_msg_send_rcv(dev_ctx, tx_msg, cmd_snd_rcv_rsp_info.tx_buf_sz,
> +			       rx_msg, cmd_snd_rcv_rsp_info.rx_buf_sz);
> +	if (err < 0) {
> +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +
> +		return err;
> +	}

[Severity: Critical]
Does this allow arbitrary physical memory read/write vulnerabilities? Since
the earlier se_chk_tx_msg_hdr() check only verifies the UAPI command header,
user-controlled payloads containing embedded physical DMA addresses are sent
directly to the hardware here without validation. An attacker could forge
addresses to overwrite kernel memory.

[Severity: High]
If ele_msg_send_rcv() successfully completed a transaction on the hardware
but was interrupted by a signal, it overwrites the success with -ERESTARTSYS.
Does returning that negative error here cause issues? This triggers the
se_ioctl_cmd_snd_rcv_cleanup() handler which zeros the shared DMA input
buffers. The VFS will then restart the ioctl and execute a second time with
all-zero input parameters.

> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
> +	struct miscdevice *miscdev = fp->private_data;
> +	struct se_if_open_gate *gate;
> +	struct se_if_device_ctx *misc_dev_ctx;
> +	struct se_if_device_ctx *dev_ctx;
> +	struct se_if_priv *priv;
> +	int err = 0;
> +
> +	gate = container_of(miscdev, struct se_if_open_gate, miscdev);
> +
> +	if (!se_if_open_gate_get(gate))
> +		return -ENODEV;

[Severity: High]
Is there a use-after-free race here against module removal?
During unbind, se_if_probe_cleanup() calls misc_deregister() and frees
the gate structure. This does not synchronize with concurrent open() calls
that have already resolved the device in the VFS, leading to a dereference
of the freed miscdev and gate when the delayed fops->open runs.

> @@ -310,6 +1467,67 @@ static void se_if_probe_cleanup(void *plat_dev)
[ ... ]
> +	while (true) {
> +		dev_ctx = NULL;
> +
> +		scoped_guard(mutex, &priv->modify_lock) {
> +			if (list_empty(&priv->dev_ctx_list))
> +				goto out_done;
> +
> +			dev_ctx = list_first_entry(&priv->dev_ctx_list,
> +						   struct se_if_device_ctx, link);
> +
> +			/* pin this context so close() cannot free it under us */
> +			kref_get(&dev_ctx->refcount);
> +			dlink_dev_ctx(dev_ctx);
> +		}
> +
> +		/*
> +		 * Local cleanup outside the global lock avoids ABBA deadlock
> +		 * with paths that already take dev_ctx->fops_lock first.
> +		 */
> +		cleanup_dev_ctx(dev_ctx, false);
> +		kref_put(&dev_ctx->refcount, se_if_dev_ctx_release);
> +	}
> +out_done:
> +
> +	/*
> +	 * A timed-out synchronous command may have retained a dev_ctx through
> +	 * priv->fw_busy_dev_ctx even after the fd was closed and the context was
> +	 * removed from dev_ctx_list. If no late response arrived, release that
> +	 * retained context during driver teardown.
> +	 *
> +	 * se_clear_fw_busy() is idempotent and internally checks
> +	 * priv->fw_busy_dev_ctx under fw_busy_lock.
> +	 */
> +	complete_all(&priv->waiting_rsp_clbk_hdl.done);

[Severity: High]
Will this cause a deadlock during driver unbind?
If an ioctl thread is blocked waiting for the done completion, it currently
holds dev_ctx->fops_lock. Calling cleanup_dev_ctx() here attempts to acquire
that same lock and blocks indefinitely, because the complete_all() intended
to wake the ioctl thread is placed after this loop.

> @@ -418,13 +1653,12 @@ static int se_if_probe(struct platform_device *pdev)
[ ... ]
> -	ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx);
> +	ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx, &se_if_fops);
>  	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.*/
>  	if (se_info->imem_state_mgmt) {
>  		load_fw = get_load_fw_instance(priv);
>  
[ ... ]
>  	if (if_node->if_defs.se_if_type == SE_TYPE_ID_HSM) {
>  		ret = get_se_soc_info(priv, se_info);

[Severity: Medium]
Could exposing the miscdevice to userspace this early lead to TOCTOU races?
The device is registered before get_se_soc_info() is queried or the IMEM
DMA buffer is allocated, allowing userspace to open the device and interact
with uninitialized state.

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