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

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

On Tue, Sep 01, 2026 at 03:52:35PM +0000, Pankaj Gupta (OSS) wrote:
> > > --- 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.
> >
>
> NOT A BUG. SE_IOCTL_ENABLE_CMD_RCV_STATUS is a status-query IOCTL, not a
> registration IOCTL. Its purpose is to report whether the calling file descriptor
> is already the registered command receiver, via check_cmd_rcvr_status().
> Registration is performed by set_dev_ctx_as_command_receiver(), called from
> fw_api_specific_ops() during the storage session open flow
> (ELE_STORAGE_OPEN_REQ). The error log message "Failed to register" is
> admittedly misleading but the underlying logic is correct. The log message
> will be improved in a follow-up cleanup patch.

Maybe error message "Failed[%ld] to register %s as CMD-Receiver", miss
leading AI.

SE_IOCTL_ENABLE_CMD_RCV_STATUS
	 ^^^   also miss leading, according to you description, look like

SE_IOCTL_GET_CMD_RCV_STATUS

Frank
>
> > [ ... ]
> > > +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].
>
> NOT A BUG. se_val_rsp_hdr_n_status() only accesses msg->header fields (tag,
> command, size, ver) and msg->data[0] (the firmware status word). The sz
> argument is used solely as a consistency check against header->size to detect
> a mismatch; it is never used to index or iterate over the buffer. No
> out-of-bounds read is possible regardless of what the firmware header claims.
>
> If userspace provides an rx_buf_sz smaller than the firmware response, the
> MU/mailbox layer silently truncates the response. This is a caller contract
> violation: the userspace library (plat_os_abs_cmd_snd_rcv_rsp()) is expected
> to allocate an rx buffer matching the maximum expected response size for the
> given command. Returning -ENOSPC in this case would orphan any
> firmware-allocated handle (the firmware command already succeeded), creating
> inconsistent state between firmware and userspace that cannot be recovered
> without a reset.
>
> The correct handling is a dev_warn diagnostic only to flag the mismatch to developers.
>
> >
> > [ ... ]
> > > +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.
> >
> BUG - FIXED in this series. You are correct. devm_mutex_init() was used for
> priv->se_if_cmd_lock and priv->modify_lock. Because priv is reference-counted
> (kref) and can outlive device unbind when a file descriptor remains open, the
> devm cleanup at unbind destroys these mutexes while they are still reachable.
> A subsequent close() would acquire a destroyed mutex - a use-after-free of
> mutex state.
>
> Fixed by replacing devm_mutex_init() with plain mutex_init() for both mutexes.
> The mutexes are embedded directly inside struct se_if_priv (not separately
> allocated) and are released automatically when kfree(priv) runs in
> se_if_priv_release() once the last kref is dropped. Explicit
> mutex_destroy() is added before kfree().
>
> > [ ... ]
> > > +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.
>
> NOT A BUG after the fix above. With the change to mutex_init() (which cannot
> fail), the early-return path at devm_mutex_init() no longer exists. There is
> no leak because mutex_init() always succeeds. No additional cleanup path is
> needed.
>
> >
> > [ ... ]
> > > +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?
>
> NOT A BUG. init_device_context() calls init_se_shared_mem(), which calls
> dma_alloc_coherent(). If the CMA/DMA pool is exhausted, dma_alloc_coherent()
> returns NULL and the error propagates as -ENOMEM back to open(), which fails.
> This naturally self-limits allocation: once CMA is exhausted, further open()
> calls fail with -ENOMEM. The kernel's CMA pool size is a system-wide tunable
> and the driver does not need to impose an additional per-device open count
> cap on top of that. No change needed.
>
> >
> > 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.
> NOT A BUG.
> Both ELE_OEM_AUTH_CONTAINER_REQ and ELE_KEYSTORE_REPROV_ENABLE_REQ pass
> read-only input buffers; firmware never writes back through these addresses.
> A rogue caller can at most supply invalid authentication data, causing the
> firmware operation to fail - not corrupt kernel memory. Firmware is
> responsible for container bounds validation. If firmware reads past the
> shared-memory window, the IOMMU will fault and the access will be terminated
> by hardware before any kernel memory is affected. The original
> SE_CMD_ADDR_NO_SIZE entries with buf_size = 0 are correct and the existing
> comment in the source already documents this design decision. No change needed.
>
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260831-imx-se-if-v43-0-
> > a3deadbda4ef@nxp.com?part=5
>
> NXP Confidential