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

> [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?
It was for debug logs.
No, it was not required.

>
> > 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?
>
Not an issue.
FW only allow one active storage handle at a time.
Even if the second wins the race, FW will send failure response, as existing active storage handle exits.
FW's single-handle enforcement is the authoritative gate. A kernel pre-check race doesn't matter because a second FW call gets a failure response; no second valid handle is issued.

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

No. As mentioned above only one active storage handle can exists.
If the set_dev_ctx_as_command_receiver() fails, the user space will come to know When it send the IOCTL: SE_IOCTL_ENABLE_CMD_RCV_STATUS.
It will also fail with same reason as set_dev_ctx_as_command_receiver() failed during registration with bool is_ioctl)  as false.
Which will initiate the cleanup of storage handle followed by session handle.
The premise of the scenario (second caller holds a valid strg_hdl while a different dev_ctx is the registered receiver) cannot occur precisely as mentioned above.
FW rejects the second STORAGE_OPEN, so `fw_api_specific_ops()` never stores a non-zero `strg_hdl` for the second caller.
The `set_dev_ctx_as_command_receiver()` error path logging is correct and the SE_IOCTL_ENABLE_CMD_RCV_STATUS mechanism gives userspace the right signal.

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

Accepted, will fix in v41.

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260822-imx-se-if-v40-0-
> [email protected]?part=5

NXP Confidential