Re: [RFC PATCH v1 1/6] mailbox: riscv-sbi-mpxy: add riscv_sbi_mpxy_mbox_call() for hart-local requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The per-hart shared memory (`mpxy->shmem`) is vulnerable to corruption by MSI hardirqs due to insufficient interrupt masking in multiple code paths.
- [High] Multiple functions evaluate `this_cpu_ptr()` before disabling preemption, risking cross-CPU memory corruption if the task is migrated.
- [High] Missing bounds check on the firmware-supplied response length against the actual shared memory size allows an out-of-bounds read of kernel memory.
- [High] Missing bounds check on firmware-supplied `returned` count in `mpxy_get_channel_ids()` leads to severe out-of-bounds read.
--

commit 47c5774a19a0d62c9200c1c19adf1be0e5a99331
Author: Marouene Boubakri <marouene.boubakri@oss.nxp.com>

mailbox: riscv-sbi-mpxy: add riscv_sbi_mpxy_mbox_call() for hart-local requests

An SBI MPXY message send is not queued in a hardware mailbox: it is an
ecall executed on the calling hart with the calling hart's shared memory
which returns once the SBI implementation has processed the message.

> diff --git a/drivers/mailbox/riscv-sbi-mpxy-mbox.c b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> index ea69c6b6b4f99..2c80e6fd19b6d 100644
> --- a/drivers/mailbox/riscv-sbi-mpxy-mbox.c
> +++ b/drivers/mailbox/riscv-sbi-mpxy-mbox.c
[ ... ]
> @@ -719,6 +720,65 @@ static const struct mbox_chan_ops mpxy_mbox_ops = {
>  	.shutdown = mpxy_mbox_shutdown,
>  };
>  
> +/**
> + * riscv_sbi_mpxy_mbox_call() - Send an RPMI message directly on an MPXY channel
> + * @chan: SBI MPXY mailbox channel owned by the caller
> + * @msg: RPMI message of type RPMI_MBOX_MSG_TYPE_SEND_WITH_RESPONSE or
> + *       RPMI_MBOX_MSG_TYPE_SEND_WITHOUT_RESPONSE
[ ... ]
> +int riscv_sbi_mpxy_mbox_call(struct mbox_chan *chan,
> +			     struct rpmi_mbox_message *msg)
> +{
> +	struct mpxy_mbox_channel *mchan;
> +	unsigned long flags;
> +
> +	if (!chan || !chan->cl || !chan->mbox || !msg)
> +		return -EINVAL;
> +	if (chan->mbox->ops != &mpxy_mbox_ops)
> +		return -EINVAL;
> +	if (msg->type != RPMI_MBOX_MSG_TYPE_SEND_WITH_RESPONSE &&
> +	    msg->type != RPMI_MBOX_MSG_TYPE_SEND_WITHOUT_RESPONSE)
> +		return -EINVAL;
> +
> +	mchan = chan->con_priv;
> +	if (mchan->attrs.msg_proto_id != SBI_MPXY_MSGPROTO_RPMI_ID)
> +		return -EOPNOTSUPP;
> +
> +	local_irq_save(flags);
> +	mpxy_mbox_send_rpmi_data(mchan, msg);

[Severity: High]
This isn't a bug introduced by this patch, but is a severe pre-existing issue
in the modified file.

The driver heavily relies on mpxy->shmem, a per-CPU shared memory buffer, for
communicating with the firmware. Existing functions such as
mpxy_get_notifications(), mpxy_write_attrs(), and
mpxy_send_message_with_resp() protect this buffer solely by disabling
preemption via get_cpu(), leaving local hardware interrupts enabled.

As explicitly noted in the patch's commit message, mpxy->shmem is also
accessed from hard interrupt context (e.g., by the RPMI system MSI irqchip
via mbox_send_message()). If a hardware interrupt fires while a thread is
executing one of these functions (for example, reading notifications in
process context), the hardirq handler will re-enter the driver on the same
CPU, overwrite the shared memory, and silently corrupt the payload of the
interrupted thread.

For example, in mpxy_send_message_with_resp():

	get_cpu();

	/* Message protocols allowed to have no data in messages */
	if (tx_len)
		memcpy(mpxy->shmem, tx, tx_len);

Preemption is disabled but hardware interrupts remain enabled. Can an incoming
hardirq re-enter the driver and corrupt the payload just written to the shared
memory?

[Severity: High]
This is a pre-existing issue, but multiple functions evaluate this_cpu_ptr()
before disabling preemption, risking cross-CPU memory corruption if the task
is migrated.

Functions like mpxy_read_attrs() and mpxy_get_notifications() evaluate
this_cpu_ptr(&mpxy_local) at the very beginning of the function while
preemption is still enabled. Preemption is only disabled later when get_cpu()
is called.

For example, in mpxy_read_attrs():

	struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
	struct sbiret sret;

	if (!mpxy->shmem_active)
		return -ENODEV;
	if (!attr_count || !attrs_buf)
		return -EINVAL;

	get_cpu();

If the thread is preempted and migrated to another CPU during this window,
will it resume on the new CPU with preemption disabled, but the mpxy pointer
still pointing to the shared memory of the old CPU? Could this completely
break per-CPU data isolation, allowing two different CPUs to concurrently
read/write the same shared memory page without any locking?

[Severity: High]
This is a pre-existing issue, but missing bounds checks on the
firmware-supplied response length against the actual shared memory size allows
an out-of-bounds read of kernel memory.

In mpxy_mbox_send_rpmi_data(), the driver fails to validate that the
caller-supplied msg->data.max_response_len does not exceed
mchan->max_xfer_len (the capacity of the shared memory page):

		if ((!msg->data.request && msg->data.request_len) ||
		    (msg->data.request && msg->data.request_len > mchan->max_xfer_len) ||
		    (!msg->data.response && msg->data.max_response_len)) {
			msg->error = -EINVAL;
			break;
		}

This unvalidated length is passed down to mpxy_send_message_with_resp() as
max_rx_len.

If the firmware executes the ecall and returns a response size in sret.value
that is less than the caller's huge max_rx_len but greater than
mpxy_shmem_size:

		rx_bytes = sret.value;
		if (rx_bytes > max_rx_len) {
			put_cpu();
			return -ENOSPC;
		}

		memcpy(rx, mpxy->shmem, rx_bytes);

Does this read out-of-bounds past the allocated mpxy->shmem page into adjacent
kernel memory, leaking data to the caller and potentially causing a page fault
if it crosses an unmapped boundary?

[Severity: High]
This is a pre-existing issue, but a missing bounds check on the
firmware-supplied returned count in mpxy_get_channel_ids() can lead to a
severe out-of-bounds read.

mpxy_get_channel_ids() reads the number of returned elements directly from the
firmware-populated shared memory:

		returned = le32_to_cpu(sdata->returned);

		count = returned < (channel_count - start_index) ?
			returned : (channel_count - start_index);
		memcpy_from_le32(&channel_ids[start_index], sdata->channel_array, count);

Neither returned nor count is validated against the physical capacity of the
shared memory page (mpxy_shmem_size).

Since channel_count is also derived from an unvalidated firmware response,
both values can be arbitrarily large. Will a malicious or malformed firmware
response cause a massive out-of-bounds read from mpxy->shmem, likely causing
a kernel page fault panic when crossing into unmapped memory?

> +	local_irq_restore(flags);
> +
> +	return msg->error;
> +}
> +EXPORT_SYMBOL_GPL(riscv_sbi_mpxy_mbox_call);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910012057.106966-1-marouene.boubakri@oss.nxp.com?part=1