Re: [PATCH RFC v2 02/10] mailbox: mpxy: add direct synchronous send

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mailbox: mpxy: interrupt re-entrancy corrupts per-CPU shared memory
- [High] mailbox: mpxy: out-of-bounds reads from shared memory due to unvalidated firmware lengths
--

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

mailbox: mpxy: add direct synchronous send

This patch implements send_data_sync() for RPMI MPXY channels, allowing 
RPMI messages to be dispatched synchronously. It factors per-CPU shared
memory acquisition into mpxy_shmem_get() and mpxy_shmem_put(), keeping
requests and their responses on the same CPU. It also adds a counterpart 
helper rpmi_mbox_send_message_sync() for the TEE transport.

> diff --git a/drivers/mailbox/riscv-sbi-mpxy-mbox.c b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> index ea69c6b6b4f99..5ca1b6d87f5c6 100644
> --- a/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> +++ b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> @@ -125,54 +125,83 @@ static DEFINE_PER_CPU(struct mpxy_local, mpxy_local);
>  static unsigned long mpxy_shmem_size;
>  static bool mpxy_shmem_init_done;
>  
> +static int mpxy_shmem_get(struct mpxy_local **out)
> +{
> +	struct mpxy_local *mpxy;
> +
> +	get_cpu();
> +	mpxy = this_cpu_ptr(&mpxy_local);

[Severity: High]
Does this leave per-CPU shared memory vulnerable to interrupt re-entrancy?

Since get_cpu() disables preemption but leaves hardware interrupts enabled,
what happens if an interrupt fires on this CPU while a request holds the
mpxy->shmem buffer?

Because the new mpxy_mbox_send_data_sync() runs with interrupts enabled, and
the async send path (e.g. from clock or power management interrupt handlers)
also drops the channel lock before calling send_data, an async message could
interrupt the synchronous send.

The async path would then re-enter mpxy_shmem_get() on the same CPU, receive
the exact same mpxy_local buffer, and silently overwrite the interrupted
task's request payload or response buffer.

Should mpxy_shmem_get() use local_irq_save() (or local_lock_irqsave())
to disable local interrupts and prevent this same-CPU re-entrancy?

[ ... ]

> @@ -182,55 +211,61 @@ static int mpxy_get_channel_ids(u32 channel_count, u32 *channel_ids)
>  		memcpy_from_le32(&channel_ids[start_index], sdata->channel_array, count);
>  		start_index += count;
>  	} while (remaining && start_index < channel_count);
> +	rc = 0;

[Severity: High]
Can firmware trigger an out-of-bounds read here?

The number of returned channels is derived from the firmware's
sdata->returned. While count is bounded by the total remaining channels
expected, it doesn't appear that count * sizeof(u32) is checked against
mpxy_shmem_size.

If a buggy or compromised firmware provides an unusually large returned value,
will memcpy_from_le32() read past the end of the mpxy->shmem page into
adjacent kernel memory?

[ ... ]

> @@ -259,8 +295,8 @@ static int mpxy_send_message_with_resp(u32 channel_id, u32 msg_id,
>  	if (rx && !sret.error) {
>  		rx_bytes = sret.value;
>  		if (rx_bytes > max_rx_len) {
> -			put_cpu();
> -			return -ENOSPC;
> +			rc = -ENOSPC;
> +			goto out;
>  		}
>  
>  		memcpy(rx, mpxy->shmem, rx_bytes);

[Severity: High]
Does this memcpy() lack a bounds check against the shared memory size?

The response size sret.value is assigned to rx_bytes, and while it is
checked to ensure it fits within the client-provided max_rx_len, it does
not appear to be checked against mpxy_shmem_size.

If a client requests a large response and firmware returns a length that
exceeds the shared memory allocation, will this read past the end of the
mpxy->shmem buffer and leak kernel memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-riscv-rpmi-tee-abi-v2-0-45939a13d223@oss.qualcomm.com?part=2