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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] se_if_fops_write() sends messages to the MU without acquiring the global se_if_cmd_lock, leading to concurrent mailbox accesses.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in ELE_STORAGE_OPEN_REQ allows concurrent ioctls to bypass the command-receiver exclusivity check.
- [High] A slab out-of-bounds read occurs in se_ioctl_cmd_snd_rcv_rsp_handler() due to passing an unvalidated firmware-declared size to downstream processors.
- [Medium] Unconditional destruction of a conditionally initialized mutex (load_fw_lock) causes a lockdep warning.
--

commit b30bba3cfa7b6a992952bef3da531322ea19e58d
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
Subject: firmware: imx: adds miscdev

This commit adds the driver for a communication interface to the NXP
secure-enclave. It enables exchanging messages with HW IP(s) like
EdgeLock Enclave via a character device for userspace applications.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260902-imx-se-if-v44-0-104658bf941a@nxp.com?part=5

> diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> new file mode 100644
> index 0000000000000..72cecc5c8e258
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_fw_api.c
[ ... ]
> +int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct se_msg_hdr *header,
> +			    u32 tx_msg_sz, u32 rx_msg_sz)
> +{
[ ... ]
> +		/*
> +		 * Reject the storage-open request when another context is
> +		 * already registered as the command receiver. If we let the
> +		 * command through, FW would allocate a new storage handle and
> +		 * start sending NVM callbacks for it; those callbacks would be
> +		 * routed to the existing receiver (process A), not to the
> +		 * caller (process B). This would let process A observe and
> +		 * tamper with process B's NVM traffic. Reject early, before
> +		 * the command reaches FW, so no handle is allocated and the
> +		 * state stays consistent.
> +		 */
> +		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;
> +		if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
> +			ret = -EINVAL;
> +		break;

[Severity: High]
Does this check fully prevent concurrent ioctls from bypassing the
command-receiver exclusivity?

The exclusivity check is performed under modify_lock, but the global
se_if_cmd_lock isn't acquired until later in ele_msg_send_rcv().
Two concurrent threads can both observe the receiver as NULL and proceed,
allowing multiple threads to concurrently dispatch the open request to the
firmware.

The second thread will fail to register but the error is ignored because
fw_api_specific_ops() returns void, leaving it with an active FW storage
session whose NVM callbacks will be silently misrouted to the first thread's
context.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 0792e5152436b..5c77dabbc4e40 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> @@ -383,28 +874,54 @@ static int se_if_probe(struct platform_device *pdev)
[ ... ]
> -	/*
> -	 * Initialize load_fw_lock before registering the misc device.
> -	 * A userspace process could open the device and trigger se_load_firmware()
> -	 * via IOCTL immediately after misc_register(), so the mutex must be ready
> -	 * before the device becomes visible.
> -	 */
>  	if (se_info->se_fw_img_nm.seco_fw_nm_in_rfs) {
>  		load_fw = get_load_fw_instance(priv);
> -		ret = devm_mutex_init(dev, &load_fw->load_fw_lock);
> -		if (ret)
> -			return dev_err_probe(dev, ret,
> -					     "Failed to init mutex: load_fw_lock.\n");
> +		mutex_init(&load_fw->load_fw_lock);
>  		load_fw->se_fw_img_nm = &se_info->se_fw_img_nm;
>  		load_fw->is_fw_tobe_loaded = true;
>  	}
[ ... ]
> +static void se_if_priv_release(struct kref *kref)
> +{
[ ... ]
> +	put_device(priv->dev);
> +	mutex_destroy(&priv->load_fw.load_fw_lock);

[Severity: Medium]
Does this unconditional destruction cause lockdep warnings on systems without
a firmware image?

In se_if_probe(), mutex_init(&load_fw->load_fw_lock) is executed
conditionally. If the SoC configuration does not specify a firmware image,
the mutex remains uninitialized memory, which triggers a lockdep warning
when mutex_destroy() is called here on module unload or device unbind.

[ ... ]
> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	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;
> +	}
[ ... ]
> +	rsp_status_err =
> +		se_val_rsp_hdr_n_status(priv, rx_msg, tx_msg->header.command,
> +					rx_msg->header.size << 2, tx_msg->header.ver);
> +
> +	if (!rsp_status_err) {
> +		/*
> +		 * The response is well formed and fully fits the caller's
> +		 * buffer, so any FW-allocated session/storage handle it carries
> +		 * (data[1]) has been delivered. Record it now, before the
> +		 * copy-out steps below. The FW has already committed the handle;
> +		 * running fw_api_specific_ops() only after a successful
> +		 * se_dev_ctx_cpy_out_data()/copy_to_user() would leave the
> +		 * handle untracked - and so never closed on teardown, leaking it
> +		 * in FW - whenever the caller supplied a bad output pointer.
> +		 */
> +		fw_api_specific_ops(dev_ctx, rx_msg, false);

[Severity: High]
Can this lead to an out-of-bounds read if the userspace-supplied buffer is
smaller than the firmware response?

rx_msg is allocated using cmd_snd_rcv_rsp_info.rx_buf_sz, but the
firmware-declared size (rx_msg->header.size << 2) is passed to
se_val_rsp_hdr_n_status() and subsequently used in fw_api_specific_ops().

This trusts the firmware-declared size instead of the clamped received size,
allowing out-of-bounds reads into msg->data[0] and rx_msg->data[1] to leak
kernel heap memory into persistent handles.

[ ... ]
> +static ssize_t se_if_fops_write(struct file *fp, const char __user *buf,
> +				size_t size, loff_t *ppos)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
> +	struct se_if_priv *priv;
> +	int err;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &dev_ctx->fops_lock) {
> +		if (dev_ctx->cleanup_done)
> +			return -ENODEV;
[ ... ]
> +		err = ele_msg_send(dev_ctx, tx_msg, size);
> +
> +		return err;
> +	}
> +}

[Severity: High]
Does this expose the shared mailbox to concurrent writes?

The function acquires the per-context fops_lock but fails to acquire the
global se_if_cmd_lock before calling ele_msg_send(), which invokes
mbox_send_message() on the shared priv->tx_chan.

Because fops_lock is per file descriptor, multiple processes can call write()
concurrently and dispatch messages simultaneously to the same channel, which
can corrupt the hardware mailbox state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-imx-se-if-v45-0-6e400879d8ec@nxp.com?part=5