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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mailbox: mpxy: data corruption in per-CPU shmem during synchronous message send
--

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

mailbox: mpxy: add direct synchronous send

Implement send_data_sync() for RPMI MPXY channels, reusing the existing
RPMI message dispatch and returning its status directly instead of going
through mbox_chan_txdone(). Factor per-hart shared-memory acquisition into
mpxy_shmem_get()/mpxy_shmem_put() so both paths pin the CPU. Add
rpmi_mbox_send_message_sync() as the RPMI counterpart.

> 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]
Since get_cpu() disables preemption but leaves local interrupts enabled,
can a hardware interrupt preempt this code and re-enter the mailbox driver?

If an interrupt handler queues an asynchronous message, it will re-enter
mpxy_shmem_get() on the same CPU. Because it receives the exact same
mpxy_local instance, could it overwrite the per-CPU buffer?

> +	if (!mpxy->shmem_active) {
> +		put_cpu();
> +		return -ENODEV;
> +	}
> +
> +	*out = mpxy;
> +	return 0;
> +}

> [ ... ]

> @@ -239,16 +274,17 @@ static int mpxy_send_message_with_resp(u32 channel_id, u32 msg_id,
>  				       void *rx, unsigned long max_rx_len,
>  				       unsigned long *rx_len)
>  {
> -	struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
> +	struct mpxy_local *mpxy;
>  	unsigned long rx_bytes;
>  	struct sbiret sret;
> +	int rc;
>  
> -	if (!mpxy->shmem_active)
> -		return -ENODEV;
>  	if (!tx && tx_len)
>  		return -EINVAL;
>  
> -	get_cpu();
> +	rc = mpxy_shmem_get(&mpxy);
> +	if (rc)
> +		return rc;
>  
>  	/* Message protocols allowed to have no data in messages */
>  	if (tx_len)
> +		memcpy(mpxy->shmem, tx, tx_len);
>  
>  	sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_SEND_MSG_WITH_RESP,
>  			 channel_id, msg_id, tx_len, 0, 0, 0);

[Severity: High]
Could the shared memory be silently corrupted here if an interrupt fires while
the CPU is executing the message send?

For example, if an interrupt occurs after copying the request but before
invoking sbi_ecall, or before reading the response, it might overwrite
mpxy->shmem with its own payload. This could lead to incorrect firmware
requests or corrupted responses for the interrupted synchronous transaction.

> [ ... ]

> @@ -516,12 +562,21 @@ static int mpxy_mbox_send_data(struct mbox_chan *chan, void *data)
>  {
>  	struct mpxy_mbox_channel *mchan = chan->con_priv;
>  
> -	if (mchan->attrs.msg_proto_id == SBI_MPXY_MSGPROTO_RPMI_ID) {
> -		mpxy_mbox_send_rpmi_data(mchan, data);
> -		return 0;
> -	}
> +	if (mchan->attrs.msg_proto_id != SBI_MPXY_MSGPROTO_RPMI_ID)
> +		return -EOPNOTSUPP;
> +
> +	mpxy_mbox_send_rpmi_data(mchan, data);
> +	return 0;
> +}
> +
> +static int mpxy_mbox_send_data_sync(struct mbox_chan *chan, void *data)
> +{
> +	struct mpxy_mbox_channel *mchan = chan->con_priv;

[Severity: High]
Does this new synchronous entry point need IRQ protection?

It appears mpxy_mbox_send_data_sync() is invoked without holding the
channel lock and executes with interrupts enabled. This would expose the
underlying per-CPU buffer logic to the concurrency issues mentioned above.

> +
> +	if (mchan->attrs.msg_proto_id != SBI_MPXY_MSGPROTO_RPMI_ID)
> +		return -EOPNOTSUPP;
>  
> -	return -EOPNOTSUPP;
> +	return mpxy_mbox_send_rpmi_data(mchan, data);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-riscv-rpmi-tee-abi-v1-0-04908b81d885@oss.qualcomm.com?part=2