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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] API Contract Violation: SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl unconditionally fails with -ENOMEM.
- [High] Race Condition / Security Isolation: TOCTOU race in ELE_STORAGE_OPEN_REQ allows bypassing receiver uniqueness.
- [High] Race Condition / Use-After-Free: `se_if_fops_read` accesses `dev_ctx->devname` without `fops_lock` during error handling.
- [Low] Code Quality / Documentation Mismatch: Missing `__func__` in error logs.
--

--- Patch [5]: [PATCH v40 5/7] firmware: imx: adds miscdev ---
commit 823b871a16858b528f46321987d0641124095c4f
Author: Pankaj Gupta <[email protected]>

firmware: imx: adds miscdev

This commit adds a miscdev interface to exchange messages with NXP secure
enclaves from userspace. It provides an ioctl interface and file operations
for memory management, getting info, and command/response handling.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=5

> - Fix dev_err format specifier from %ld to %d for the err variable and
>   add __func__ to CMD-Receiver registration failure messages.

[Severity: Low]
The commit message states that __func__ was added to the CMD-Receiver
registration failure messages. Looking at the code in fw_api_specific_ops(),
the dev_err() call for registration failure doesn't appear to include
__func__. Did this change get missed in the final version of the patch?

> diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> --- /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)
> +{
[ ... ]
> +	case ELE_STORAGE_OPEN_REQ:
> +		/* Might be cleared as part of tear down. */
> +		if (dev_ctx->strg_hdl) {
> +			ret = -EEXIST;
> +			break;
> +		}
> +		/*
> +		 * Reject the storage-open request when another context is
> +		 * already registered as the command receiver.
[ ... ]
> +		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]
Could this check be vulnerable to a race condition? Since the global
se_if_cmd_lock is not held during this validation, two concurrent threads
issuing an ELE_STORAGE_OPEN_REQ could potentially both pass this check.

If that happens, thread A would acquire the lock, send its command, and
register as the receiver. Thread B would then acquire the lock, send its
command, allocate a second handle, but fail registration in
fw_api_specific_ops(). Because that failure is only logged and ignored,
does thread B successfully open storage but have its callbacks silently
routed to thread A?

> 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
[ ... ]
> +int set_dev_ctx_as_command_receiver(struct se_if_device_ctx *dev_ctx, bool is_ioctl)
> +{
> +	struct se_if_priv *priv = dev_ctx->priv;
> +	struct se_api_msg *new_rx_msg = NULL;
> +	struct se_clbk_handle *se_clbk_hdl;
> +	unsigned long flags;
> +
> +	se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
> +	guard(mutex)(&priv->modify_lock);
> +	if (se_clbk_hdl->dev_ctx == dev_ctx)
> +		return 0;
> +
> +	if (se_clbk_hdl->dev_ctx)
> +		return -EBUSY;
> +
> +	if (!dev_ctx->strg_hdl)
> +		return -EINVAL;
> +
> +	if (is_ioctl)
> +		return -ENOMEM;

[Severity: High]
Is this intended to unconditionally fail the ioctl with an out-of-memory
error?

When the SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl is invoked in se_ioctl(), it
calls this function with is_ioctl set to true, which will immediately return
-ENOMEM. Does this make it impossible for userspace to register as a
command receiver via the documented ioctl?

[ ... ]
> +static ssize_t se_if_fops_read(struct file *fp, char __user *buf, size_t size,
> +			       loff_t *ppos)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
[ ... ]
> +	err = ele_msg_rcv(dev_ctx, &priv->cmd_receiver_clbk_hdl);
> +	if (err < 0) {
> +		if (err != -ERESTARTSYS)
> +			dev_err(priv->dev,
> +				"%s: Er[0x%x]: Signal Interrupted. Current act-dev-ctx count: %d.",
> +				dev_ctx->devname, err, dev_ctx->priv->active_devctx_count);
> +		return err;
> +	}

[Severity: High]
Could accessing dev_ctx->devname here lead to a use-after-free?

The fops_lock is explicitly dropped before calling ele_msg_rcv() above. If
a concurrent unbind occurs and tears down the context, cleanup_dev_ctx()
could run, acquire fops_lock, and free dev_ctx->devname. If ele_msg_rcv()
then returns an error, this error path will execute without fops_lock and
dereference the potentially freed dev_ctx->devname pointer.

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