Re: [PATCH v3 4/4] firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] devm_request_any_context_irq() positive success values are incorrectly rejected by strict `ret == 0` check.
- [High] Dropping trx_list_lock before callback execution allows client stack popping, leading to UAF/stack corruption.
- [Medium] Improper handling of -EPROBE_DEFER from of_irq_get().
--

--- Patch [4]: [PATCH v3 4/4] firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt ---
commit 0c0971a9e6fe55c0bfa458e38981b259f988e3ae
Author: Mahesh Rao <mahesh.rao@altera.com>

firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt

Add support for SDM mailbox doorbell interrupt for async transactions. On
interrupt, a workqueue is triggered which polls the ATF for pending responses
and retrieves the bitmap of all retrieved and unprocessed transaction ids of
mailbox responses from SDM. It then triggers the corresponding registered
callbacks.

> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index f8c2da207cb4e..8c86441eae58b 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c

[ ... ]

> @@ -1659,14 +1666,96 @@ static inline void stratix10_smc_1_2(struct stratix10_async_ctrl *actrl,
>  	arm_smccc_1_2_smc(args, res);
>  }
>  
[ ... ]
> +static void stratix10_async_workqueue_handler(struct work_struct *work)
> +{
[ ... ]
> +		do {
> +			transaction_id = find_next_bit(pend_on_irq,
> +						       TOTAL_TRANSACTION_IDS,
> +						       transaction_id);
> +			if (transaction_id >= TOTAL_TRANSACTION_IDS)
> +				break;
> +
> +			cb = NULL;
> +			cb_arg = NULL;
> +			spin_lock(&actrl->trx_list_lock);
> +			hash_for_each_possible(actrl->trx_list, handler,
> +					       next, transaction_id) {
> +				if (handler->transaction_id == transaction_id) {
> +					cb = handler->cb;
> +					cb_arg = handler->cb_arg;
> +					break;
> +				}
> +			}
> +			spin_unlock(&actrl->trx_list_lock);
> +			if (cb)
> +				cb(cb_arg);

[Severity: High]
Is there a risk of stack corruption or a use-after-free here?

By executing the callback after dropping trx_list_lock, a racing client
timeout might execute stratix10_svc_async_done() and delete the handler.

This could result in popping the client stack before the workqueue executes
the callback on the now-invalid stack address cb_arg. Should the lock be held 
during the callback or is there another way to ensure cb_arg remains valid?

> +
> +			transaction_id++;
> +		} while (transaction_id < TOTAL_TRANSACTION_IDS);
> +	}
> +	enable_irq(actrl->irq);
> +}

[ ... ]

> @@ -1727,6 +1816,22 @@ static int stratix10_svc_async_init(struct stratix10_svc_controller *controller)
>  	hash_init(actrl->trx_list);
>  	atomic_set(&actrl->common_achan_refcount, 0);
>  
> +	irq = of_irq_get(dev_of_node(dev), 0);
> +	if (irq <= 0) {

[Severity: Medium]
Does this improperly swallow -EPROBE_DEFER?

Because of_irq_get() can return -EPROBE_DEFER as a negative value,
this check will convert the probe deferral into a permanent fallback
to polling mode. Should this propagate -EPROBE_DEFER instead?

> +		dev_warn(dev, "Failed to get IRQ, falling back to polling mode\n");
> +	} else {
> +		ret = devm_request_any_context_irq(dev, irq, stratix10_svc_async_irq_handler,
> +						   IRQF_NO_AUTOEN, "stratix10_svc", controller);
> +		if (ret == 0) {

[Severity: High]
Will this correctly handle nested IRQs?

devm_request_any_context_irq() returns IRQC_IS_NESTED (which is 1) on success 
for nested IRQs.

By strictly checking for ret == 0, successful nested IRQ assignments will
be rejected. This skips INIT_WORK and enable_irq(), leaving the registered IRQ
permanently disabled and causing asynchronous transactions to time out. 
Should this check for ret >= 0 instead?

> +			dev_info(dev,
> +				 "Registered IRQ %d for sip async operations\n",
> +				 irq);
> +			actrl->irq = irq;
> +			INIT_WORK(&actrl->async_work, stratix10_async_workqueue_handler);
> +			enable_irq(actrl->irq);
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786600929.git.tze.yee.ng@altera.com?part=4