Re: [PATCH RFC 4/5] optee: riscv: add asynchronous notifications over the signal bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Reusing the `rx` buffer without validating `msg.data.out_response_len` allows stale data to trigger an infinite loop in the threaded IRQ handler.
- [High] Failing to drain pending signals on error paths (such as dynamic memory allocation failure) leads to an interrupt storm.
- [Medium] Swallowing `-EPROBE_DEFER` from `platform_get_irq_optional` permanently disables async notifications on unfavorable driver probe orders.
--

commit 0300738ec4be7419d301effc706a11f0f885fbcb
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

optee: riscv: add asynchronous notifications over the signal bus

This commit adds asynchronous notification support using the RPMI TEE signal
bus. It sets up a signal bus between OP-TEE and Linux, using the platform
interrupt as the availability doorbell.

> diff --git a/drivers/tee/optee/optee_riscv.c b/drivers/tee/optee/optee_riscv.c
> index 72c9eb85f4fed..b9bb813adef50 100644
> --- a/drivers/tee/optee/optee_riscv.c
> +++ b/drivers/tee/optee/optee_riscv.c
[ ... ]
> @@ -852,6 +856,242 @@ static int optee_riscv_do_call_with_arg(struct tee_context *ctx,
>  	return optee_riscv_yielding_call(ctx, in, rpc_arg, system_thread);
>  }
>  
> +/*
> + * 5b. Asynchronous notification over the signal bus
[ ... ]
> +static bool optee_riscv_retrieve_signals(struct optee *optee)
> +{
> +	bool do_bottom_half = false;
> +	size_t max_signals = optee->riscv.sender_signals;
> +	struct rpmi_tee_signal_retrieve_resp *rx;
> +	struct rpmi_mbox_message msg;
> +	size_t rx_len;
> +	u32 flags;
> +
> +	rx_len = struct_size(rx, signal, max_signals);
> +	rx = kzalloc(rx_len, GFP_KERNEL);
> +	if (!rx)
> +		return false;

[Severity: High]
What happens to the level-triggered hardware interrupt if this memory
allocation fails?

If kzalloc() fails, we abort without draining the pending signals from the
firmware. However, looking at notif_irq_handler() below, it will
unconditionally return IRQ_HANDLED. Will this unmask an uncleared
level-triggered interrupt, causing an immediate interrupt storm?

> +
> +	do {
> +		u32 status, n, i;
> +
> +		rpmi_mbox_init_send_with_response(&msg,
> +						  RPMI_TEE_SRV_SIGNAL_RETRIEVE,
> +						  NULL, 0, rx, rx_len);
> +		if (optee_riscv_send(optee, &msg))
> +			break;
> +
> +		status = le32_to_cpu(rx->status);

[Severity: High]
Does this loop properly validate the actual payload length of the response?

Since the rx buffer is reused in this loop, if the RPMI firmware returns a
truncated or empty response, it seems we might read stale fields (like
status, signal_len, and flags) left over from the previous iteration.

Could this lead to an infinite loop if the stale flags still contain the
RPMI_TEE_SIGNAL_RETRIEVE_MORE_AVAILABLE bit?

> +		if (status == (u32)RPMI_ERR_NO_DATA)
> +			break;
> +		if (status)
> +			break;
> +
> +		n = min_t(u32, le32_to_cpu(rx->signal_len), max_signals);
> +		for (i = 0; i < n; i++) {
> +			u32 value = le32_to_cpu(rx->signal[i]);
> +
> +			if (value == OPTEE_ABI_ASYNC_NOTIF_BOTTOM_HALF)
> +				do_bottom_half = true;
> +			else
> +				optee_notif_send(optee, value);
> +		}
> +
> +		flags = le32_to_cpu(rx->flags);
> +	} while (flags & RPMI_TEE_SIGNAL_RETRIEVE_MORE_AVAILABLE);
> +
> +	kfree(rx);
> +
> +	return do_bottom_half;
> +}
> +
> +static irqreturn_t notif_irq_handler(int irq, void *dev_id)
> +{
> +	struct optee *optee = dev_id;
> +
> +	if (optee_riscv_retrieve_signals(optee))
> +		queue_work(optee->riscv.notif_wq, &optee->riscv.notif_work);
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> @@ -1291,6 +1532,13 @@ static int optee_riscv_probe(struct platform_device *pdev)
>  	if (rc)
>  		goto err_close_ctx;
>  
> +	if (sec_caps & OPTEE_ABI_SEC_CAP_ASYNC_NOTIF) {
> +		rc = optee_riscv_async_notif_init(pdev, optee);
> +		if (rc)
> +			dev_warn(dev, "Failed to initialize async notifications: %d\n",
> +				 rc);
> +	}

[Severity: Medium]
Is it intentional to swallow errors from optee_riscv_async_notif_init() here?

If the interrupt controller driver is probed after OP-TEE,
platform_get_irq_optional() inside optee_riscv_async_notif_init() will return
-EPROBE_DEFER.

By swallowing this error code instead of returning it to the driver core,
won't we permanently disable async notifications for this boot session on
unfavorable probe orders?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-rpmi-tee-service-grp-dev-v1-0-1d1d35c2a859@oss.qualcomm.com?part=4