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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl fails to actually register and arm the command receiver, effectively breaking the ioctl's functionality.
- [High] Bypassing the user-provided buffer size when validating the firmware response causes out-of-bounds reads on the rx_msg allocation and fails to capture the handle.
- [High] The `priv->modify_lock` and `priv->se_if_cmd_lock` mutexes are accessed after being destroyed by devres on device unbind, leading to a Use-After-Free of mutex state.
- [High] Hardware out-of-bounds read via missing DMA buffer size boundary checks for variable-length input buffers.
- [High] Unbounded DMA coherent memory allocation in `se_if_fops_open()` allows userspace to exhaust CMA/DMA pools.
- [Medium] The `se_if_probe` function leaks the `priv` structure and a `struct device` reference if `devm_mutex_init` fails.
--

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

This commit adds a character driver to enable exchanging messages with the
NXP secure enclave hardware from userspace. It introduces ABI definitions
and base API checks to block arbitrary command exchanges.

> 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
[ ... ]
> +		switch (cmd) {
> +		case SE_IOCTL_ENABLE_CMD_RCV_STATUS: {
> +			guard(mutex)(&priv->modify_lock);
> +			err = check_cmd_rcvr_status(dev_ctx);
> +			if (err)
> +				dev_err(priv->dev, "Failed[%ld] to register %s as CMD-Receiver.\n",
> +					err, dev_ctx->devname);
> +		break;
> +		}

[Severity: High]
Is it intentional that se_ioctl() only checks the status here but does not
actually perform the registration? 

It appears set_dev_ctx_as_command_receiver() is never called, causing the
ioctl to fail its intended purpose and always return -ENXIO if not already
registered.

[ ... ]
> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	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 == priv->if_defs->base_api_ver);

[Severity: High]
Could passing the firmware-declared size (rx_msg->header.size << 2) instead of
the caller's allocated size (rx_buf_sz) lead to out-of-bounds reads? 

If a user submits an intentionally small rx_buf_sz, the subsequent validation
in se_val_rsp_hdr_n_status() and fw_api_specific_ops() might read past the
bounds of the allocated rx_msg buffer when accessing data[0] or data[1].

[ ... ]
> +static int se_if_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = devm_mutex_init(dev, &priv->se_if_cmd_lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to init mutex: priv se_if_cmd_lock.\n");
> +	ret = devm_mutex_init(dev, &priv->modify_lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to init mutex: priv modify_lock.\n");

[Severity: High]
Does binding the lifecycle of these mutexes to device unbind create a 
use-after-free risk? 

Since priv is refcounted and can outlive device unbind (e.g., if a file 
descriptor remains open), closing the file later calls dlink_n_cleanup_dev_ctx()
and cleanup_dev_ctx(), which will attempt to acquire these destroyed mutexes.

[ ... ]
> +static int se_if_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	priv = kzalloc_obj(*priv);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	/*
> +	 * Pin the parent device for the lifetime of priv. A file descriptor may
> +	 * stay open after the device is unbound; close() then still passes
> +	 * priv->dev to dma_free_coherent()/dev_warn(). Without this reference
> +	 * the struct device could be freed while priv->dev still points at it,
> +	 * so the reference is dropped in se_if_priv_release() via put_device().
> +	 */
> +	get_device(priv->dev);
> +	kref_init(&priv->refcount);
> +	priv->if_defs = &if_node->if_defs;
> +	dev_set_drvdata(dev, priv);
> +
[ ... ]
> +	ret = devm_mutex_init(dev, &priv->se_if_cmd_lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to init mutex: priv se_if_cmd_lock.\n");

[Severity: Medium]
If devm_mutex_init() fails, does this function return directly without
cleaning up? 

It appears this would leak the priv structure and leave the device reference
unreleased, since devm_add_action_or_reset() has not been registered yet.

[ ... ]
> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
[ ... ]
> +	priv->dev_ctx_mono_count++;
> +	err = init_device_context(priv, priv->dev_ctx_mono_count, &dev_ctx);
> +	if (err) {
> +		dev_err(priv->dev, "Failed to create dev-ctx.\n");
> +		goto out_unlock_misc;
> +	}

> +static int init_se_shared_mem(struct se_if_device_ctx *dev_ctx)
> +{
[ ... ]
> +	se_shared_mem_mgmt->non_secure_mem.ptr =
> +			dma_alloc_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
> +					   &se_shared_mem_mgmt->non_secure_mem.dma_addr,
> +					   GFP_KERNEL);

[Severity: High]
Does this code allow unbounded DMA coherent memory allocation? 

If a userspace process repeatedly calls open() in a loop, it seems it would 
allocate 128KB each time without any limit, which could quickly exhaust 
contiguous DMA memory.

> diff --git a/drivers/firmware/imx/ele_msg_addr_field.c b/drivers/firmware/imx/ele_msg_addr_field.c
> --- a/drivers/firmware/imx/ele_msg_addr_field.c
> +++ b/drivers/firmware/imx/ele_msg_addr_field.c
[ ... ]
> +static const struct se_cmd_addr_field ele_oem_auth_cntr_addr_fields[] = {
> +	/*
> +	 * Container Header address: a 64-bit physical address split across two
> +	 * words. data[0] holds the 32-bit MSB and data[1] holds the 32-bit LSB
> +	 * (ELE API spec Table 27, word size = 0x3, so the command is header +
> +	 * MSB + LSB only). The message carries no length word for this buffer;
> +	 * the container size is variable and not communicated in the MU payload,
> +	 * and no static firmware-defined maximum is specified. Because this is a
> +	 * read-only input buffer (the ELE ROM/FW copies the container header
> +	 * into its internal memory for authentication and does not write back
> +	 * through this address), enforcing only the start-address range check is
> +	 * acceptable: a rogue caller can at most cause firmware to read within
> +	 * the shared-memory window, which is memory the caller already owns.
> +	 * Output buffers must be fully bounded; input-only buffers are safe with
> +	 * addr-only checks.
> +	 */
> +	{ .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx = SE_CMD_ADDR_ALWAYS,
> +	  .size_idx = SE_CMD_ADDR_NO_SIZE },			/* container_hdr_addr */
> +};

[Severity: High]
Can the lack of size validation here allow the firmware to read past the bounds
of the allocated DMA buffer? 

If a user submits a command with an address pointing to the very end of the
shared memory window, the firmware might read unmapped or unowned memory,
potentially causing an IOMMU fault.

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