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

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Jun 29, 2026 at 07:21:34AM +0000, sashiko-bot@kernel.org wrote:
> 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.

This is real problem, may user application use 32bit application.

>
> > 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.

Please double this one? Please look other sashiko results.

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