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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The firmware-busy circuit breaker can be permanently disabled if a recovery command times out.
--

commit 8efdce1526b5b7fd05d241f2ae2a7a89f4376e43
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: adds miscdev

Adds the driver for communication interface to secure-enclave, that
enables exchanging messages with NXP secure enclave HW IP(s)
like EdgeLock Enclave.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index b5d3421c09878..519f24d24a859 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> +static void se_clear_fw_busy(struct se_if_priv *priv)
> +{
> +	struct fw_busy_info *fbusy_info = &priv->fw_busy_info;
> +	struct se_if_device_ctx *dev_ctx = NULL;
> +	unsigned long flags;
> +
> +	scoped_guard(spinlock_irqsave, &fbusy_info->fw_busy_lock) {
> +		dev_ctx = fbusy_info->fw_busy_dev_ctx;
> +		fbusy_info->fw_busy_dev_ctx = NULL;
> +
> +		if (!dev_ctx) {
[ ... ]
> +			atomic_set(&fbusy_info->fw_busy, 0);
> +			return;
> +		}
> +	}
[ ... ]
> +	scoped_guard(mutex, &dev_ctx->fops_lock) {
[ ... ]
> +		if (have_snap) {
[ ... ]
> +			fw_api_specific_ops(priv->priv_dev_ctx,
> +					    (struct se_api_msg *)late_rx_snap, true);
[ ... ]
> +	se_release_msg_if(priv);
> +
> +	spin_lock_irqsave(&fbusy_info->fw_busy_lock, flags);
> +	atomic_set(&fbusy_info->fw_busy, 0);
> +	spin_unlock_irqrestore(&fbusy_info->fw_busy_lock, flags);

[Severity: High]
Can the circuit breaker be permanently bypassed if a recovery command times
out?

If fw_api_specific_ops() triggers a cleanup command like se_close_session()
and the firmware is still hung, that command will time out and call
se_mark_fw_busy(). Since fbusy_info->fw_busy_dev_ctx was set to NULL at the
start of this function, se_mark_fw_busy() will claim the slot again and set
fw_busy = 1.

When se_clear_fw_busy() finishes, it unconditionally sets fw_busy to 0
here, but leaves the newly armed fw_busy_dev_ctx populated. This leaves
the system in a state where fw_busy = 0 but fw_busy_dev_ctx != NULL.

If this happens, could subsequent timeouts from user-space ioctls fail to
re-arm the circuit breaker because se_mark_fw_busy() sees a non-NULL
fw_busy_dev_ctx and silently returns? This would permanently bypass the
fast reject mechanism.

> +	kref_put(&dev_ctx->refcount, se_if_dev_ctx_release);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-imx-se-if-v51-0-4a7dac612cb5@nxp.com?part=5