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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The allocated DMA address for the encrypted `imem` buffer is discarded, causing firmware to corrupt physical address 0x0.
- [High] Command receiver device contexts bypass the response timeout, causing indefinite hangs on firmware communication.
- [High] `dlink_dev_ctx()` frees the command receiver's `rx_msg` without holding the required synchronization lock.
- [High] Missing `kref_init()` in `init_device_context()` causes the file context to be permanently leaked upon close.
- [High] Unbinding the device or removing the module hangs indefinitely if a userspace thread is blocked in `read()`.
- [High] DMA memory is freed when a file descriptor is closed, even if the firmware is still actively processing a timed-out transaction using that memory.
- [High] Concurrent ioctls can cause multiple firmware authentications due to an unprotected state variable.
- [High] Kernel pointer leakage to userspace due to struct type confusion in `get_se_soc_id()`.
- [High] Early return upon mailbox send failure leaves a dangling pointer in the shared response waiter state, leading to a Use-After-Free in the RX interrupt handler.
- [High] Invalid use of `dma_to_phys()` on a DMA address returned by `dma_alloc_coherent()`.
- [Medium] UAPI ioctl structures use variable-sized types and pointers, breaking 32-bit userspace compatibility.
- [Medium] Unbounded user-provided size is passed to memdup_user(), allowing potential kernel warnings and memory exhaustion.

Pre-existing issues:
- [High] Misdirected signal handling in `ele_msg_rcv()` corrupts another thread's state and traps daemon read threads in an uninterruptible wait.
--

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

This commit adds a miscdev character driver for communicating with NXP
secure enclaves (like EdgeLock). It provides ioctls for shared memory
management, command exchange from userspace, and firmware loading lifecycle
management.

> diff --git a/include/uapi/linux/se_ioctl.h b/include/uapi/linux/se_ioctl.h
> --- /dev/null
> +++ b/include/uapi/linux/se_ioctl.h
> @@ -0,0 +1,97 @@
[ ... ]
> +struct se_ioctl_setup_iobuf {
> +	void __user *user_buf;
> +	__u32 length;
> +	__u32 flags;
> +	__u64 ele_addr;
> +};

[Severity: Medium]
Does this struct break 32-bit userspace compatibility on 64-bit kernels?

Using pointer types like void __user * changes the struct size between
architectures. Since the ioctl uses _IOWR which encodes the size, a 32-bit
process will have a mismatched size.

> 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
[ ... ]
> +	if (dev_ctx->priv->cmd_receiver_clbk_hdl.dev_ctx != dev_ctx) {
> +		is_rsp_wait_with_timeout = true;
> +		timeout_jiffies = msecs_to_jiffies(dev_ctx->priv->se_rcv_msg_timeout_ms);
> +		deadline_jiffies = jiffies + timeout_jiffies;
> +	}
>  	do {
> +		if (is_rsp_wait_with_timeout) {

[Severity: High]
Does this logic correctly identify the wait context?

It disables the timeout based on the file descriptor's identity as a command
receiver rather than the type of wait being performed. If the daemon issues
a synchronous command via SE_IOCTL_CMD_SEND_RCV_RSP, it will use
MAX_SCHEDULE_TIMEOUT and could hang indefinitely if the firmware drops a
response.

[ ... ]
> +		if (ret == -ERESTARTSYS) {
[ ... ]
> +			if (dev_ctx->priv->waiting_rsp_clbk_hdl.dev_ctx) {
> +				dev_ctx->priv->waiting_rsp_clbk_hdl.signal_rcvd = true;
> +				wait_uninterruptible = true;
> +				continue;
> +			}

[Severity: High]
This is a pre-existing issue, but does this signal handling code modify the
wrong thread's state?

When the daemon receives a signal, the hardcoded check against
priv->waiting_rsp_clbk_hdl could corrupt another concurrently waiting
thread's state. Would this force the daemon read thread into an infinite
wait_uninterruptible loop?

> 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
[ ... ]
> +static u32 get_se_soc_id(struct se_if_priv *priv)
> +{
> +	const struct se_soc_info *se_info = device_get_match_data(priv->dev);
> +
> +	return se_info->soc_id;
> +}

[Severity: High]
Does this cause a struct type confusion that leaks kernel pointers to
userspace?

device_get_match_data() returns a pointer to struct se_if_node *, but this
code mistakenly casts it to struct se_soc_info *. Reading se_info->soc_id
will extract bits from the first member of struct se_if_node, which is a
kernel pointer.

These bits are then exposed to userspace by the SE_IOCTL_GET_SOC_INFO ioctl.

[ ... ]
> +static int load_firmware(struct se_if_priv *priv, const u8 *se_img_file_to_load)
> +{
[ ... ]
> +	se_fw_buf = dma_alloc_coherent(priv->dev, fw->size, &se_fw_dma_addr, GFP_KERNEL);
[ ... ]
> +	se_fw_phyaddr = dma_to_phys(priv->dev, se_fw_dma_addr);
> +	ret = ele_fw_authenticate(priv, se_fw_phyaddr, se_fw_phyaddr);

[Severity: High]
Is it safe to use dma_to_phys() on an address returned by
dma_alloc_coherent()?

dma_alloc_coherent() returns a DMA address (IOVA on systems with an IOMMU),
and dma_to_phys() cannot reverse an IOMMU mapping. This will result in a
garbage physical address being passed to the firmware for authentication.

[ ... ]
> +static int se_load_firmware(struct se_if_priv *priv)
> +{
> +	struct se_fw_load_info *load_fw = get_load_fw_instance(priv);
> +	int ret = 0;
> +
> +	if (!load_fw->is_fw_tobe_loaded)
> +		return 0;

[Severity: High]
Is the load_fw->is_fw_tobe_loaded initialization state safe from race
conditions?

It is checked and modified here without global locking. Could concurrent
ioctls on different file descriptors bypass the boolean check
simultaneously, leading to duplicate memory allocations and multiple
conflicting firmware authentication commands?

[ ... ]
> +static void cleanup_se_shared_mem(struct se_if_device_ctx *dev_ctx)
> +{
[ ... ]
> +	/* Free non-secure shared buffer. */
> +	dma_free_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
> +			  se_shared_mem_mgmt->non_secure_mem.ptr,
> +			  se_shared_mem_mgmt->non_secure_mem.dma_addr);

[Severity: High]
If a synchronous command sent via SE_IOCTL_CMD_SEND_RCV_RSP times out, the
background firmware operation is not canceled.

When userspace closes the file descriptor, these coherent DMA buffers are
freed. Will the firmware eventually write its response to the freed address,
causing kernel memory corruption?

[ ... ]
> +static void dlink_dev_ctx(struct se_if_device_ctx *dev_ctx)
> +{
[ ... ]
> +	if (priv->cmd_receiver_clbk_hdl.dev_ctx == dev_ctx) {
> +		kfree(priv->cmd_receiver_clbk_hdl.rx_msg);
> +		priv->cmd_receiver_clbk_hdl.rx_msg = NULL;
> +		priv->cmd_receiver_clbk_hdl.dev_ctx = NULL;
> +	}

[Severity: High]
Can this result in a use-after-free and data race?

This frees the command receiver's rx_msg without acquiring the clbk_rx_lock.
Concurrently, the mailbox interrupt handler se_if_rx_callback() could
acquire the lock, observe a non-null pointer, and copy incoming firmware
data into the freed memory.

[ ... ]
> +static void cleanup_dev_ctx(struct se_if_device_ctx *dev_ctx, bool is_fclose)
> +{
> +	scoped_guard(mutex, &dev_ctx->fops_lock) {

[Severity: High]
Can this cause a deadlock during device unbind or module removal?

If a userspace daemon is blocked indefinitely in read() holding the
dev_ctx->fops_lock, the teardown path unconditionally attempts to acquire the
same fops_lock here and will hang indefinitely.

[ ... ]
> +static int init_device_context(struct se_if_priv *priv, int ch_id,
> +			       struct se_if_device_ctx **new_dev_ctx)
> +{
> +	struct se_if_device_ctx *dev_ctx;
> +	int ret = 0;
> +
> +	dev_ctx = kzalloc_obj(*dev_ctx, GFP_KERNEL);
[ ... ]
> +	mutex_init(&dev_ctx->fops_lock);
> +	dev_ctx->priv = priv;
> +	dev_ctx->cleanup_done = false;
> +	INIT_LIST_HEAD(&dev_ctx->link);
> +	*new_dev_ctx = dev_ctx;

[Severity: High]
Should this allocation include a call to kref_init(&dev_ctx->refcount)?

Without initialization, the refcount remains at 0. When close() is called,
kref_put() will fail, bypassing the se_if_dev_ctx_release callback and
permanently leaking the file context.

[ ... ]
> +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(cmd_snd_rcv_rsp_info.tx_buf,
> +			    cmd_snd_rcv_rsp_info.tx_buf_sz);

[Severity: Medium]
Is the tx_buf_sz parameter validated against an upper bound before calling
memdup_user()?

If a user process passes an extremely large value, could this lead to a
local denial of service via memory exhaustion or warning spam?

[ ... ]
> +	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);

[Severity: High]
If ele_msg_send() fails internally within ele_msg_send_rcv(), does it leave
a dangling pointer?

If ele_msg_send_rcv() returns early on error, it does not reset the shared
priv->waiting_rsp_clbk_hdl.rx_msg pointer to NULL. The caller then frees the
buffer, and a late firmware response handled by se_if_rx_callback() will
trigger a use-after-free when copying into the freed memory.

[ ... ]
> +static ssize_t se_if_fops_write(struct file *fp, const char __user *buf,
> +				size_t size, loff_t *ppos)
> +{
[ ... ]
> +		struct se_api_msg *tx_msg __free(kfree) = memdup_user(buf, size);

[Severity: Medium]
Is the size parameter validated against an upper bound before calling
memdup_user() here?

[ ... ]
> +static const struct file_operations se_if_fops = {
> +	.open		= se_if_fops_open,
> +	.owner		= THIS_MODULE,
> +	.release	= se_if_fops_close,
> +	.unlocked_ioctl = se_ioctl,
> +	.read		= se_if_fops_read,
> +	.write		= se_if_fops_write,
> +};

[Severity: Medium]
Is a .compat_ioctl handler missing here?

Because the ioctl structures use pointer types, a translation handler is
required to support 32-bit userspace binaries.

[ ... ]
> +static int se_if_probe(struct platform_device *pdev)
> +{
[ ... ]
> +		if (load_fw->se_fw_img_nm->prim_fw_nm_in_rfs) {
> +			/* allocate buffer where SE store encrypted IMEM */
> +			imem_dma_addr = phys_to_dma(priv->dev, load_fw->imem.phyaddr);
> +			load_fw->imem.buf = dmam_alloc_coherent(priv->dev, ELE_IMEM_SIZE,
> +								&imem_dma_addr, GFP_KERNEL);

[Severity: Critical]
Does this discard the DMA address allocated by dmam_alloc_coherent()?

The function returns the IOVA in the local stack variable imem_dma_addr, but
this address is never saved back to load_fw->imem.phyaddr, leaving it at 0.

The firmware might then be instructed to use physical address 0, destroying
data at page 0. This will also cause the cleanup path to pass 0 to
dmam_free_coherent().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-imx-se-if-v26-0-146446285744@nxp.com?part=5