Re: [PATCH v10 2/9] riscv: add support for SBI Supervisor Software Events extension

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Physical address truncation on RV32 with 64-bit physical addresses during SBI_SSE_EVENT_ATTR_READ/WRITE.
--

commit 009ca408c0e515d2b163c5c6737a07a0e84156e8
Author: Clément Léger <cleger@rivosinc.com>

riscv: add support for SBI Supervisor Software Events extension

This commit adds support for the SBI Supervisor Software Events (SSE)
extension, allowing firmware to notify supervisor software of independent
events. It provisions per-event stacks, saves and restores interrupted
contexts, and handles nested exceptions safely.

> diff --git a/arch/riscv/kernel/sbi_sse.c b/arch/riscv/kernel/sbi_sse.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/riscv/kernel/sbi_sse.c

[ ... ]

> +void noinstr do_sse(struct sse_event_arch_data *arch_evt,
> +		    struct pt_regs *regs, unsigned long hstatus)
> +{
> +	struct riscv_sse_interrupted_context context = { regs, hstatus };
> +	struct riscv_sse_interrupted_context *previous;
> +	struct sbiret sret;
> +
> +	nmi_enter();
> +	instrumentation_begin();
> +
> +	/* Retrieve missing GPRs from SBI */
> +	sret = sbi_ecall(SBI_EXT_SSE, SBI_SSE_EVENT_ATTR_READ, arch_evt->evt_id,
> +			 SBI_SSE_ATTR_INTERRUPTED_A6,
> +			 (SBI_SSE_ATTR_INTERRUPTED_A7 -
> +			  SBI_SSE_ATTR_INTERRUPTED_A6) + 1,
> +			 (unsigned long)arch_evt->interrupted_phys, 0, 0);

[Severity: High]
Does casting arch_evt->interrupted_phys to unsigned long truncate the
physical address on RV32 systems?

When CONFIG_PHYS_ADDR_T_64BIT is enabled on RV32, virt_to_phys() returns a
64-bit physical address. By casting to unsigned long for the phys_lo
argument and hardcoding 0 for the phys_hi argument, if the physical memory
resides above the 4GB boundary, the firmware receives a truncated 32-bit
address. Could this result in the firmware reading from unintended memory?

> +	if (sret.error) {
> +		pr_warn("Failed to read interrupted registers for event %x: %ld\n",
> +			arch_evt->evt_id, sret.error);

[ ... ]

> +	if (memcmp(&regs->a6, arch_evt->interrupted,
> +		   sizeof(*arch_evt->interrupted))) {
> +		memcpy(arch_evt->interrupted, &regs->a6,
> +		       sizeof(*arch_evt->interrupted));
> +		sret = sbi_ecall(SBI_EXT_SSE, SBI_SSE_EVENT_ATTR_WRITE,
> +				 arch_evt->evt_id, SBI_SSE_ATTR_INTERRUPTED_A6,
> +				 (SBI_SSE_ATTR_INTERRUPTED_A7 -
> +				  SBI_SSE_ATTR_INTERRUPTED_A6) + 1,
> +				 (unsigned long)arch_evt->interrupted_phys, 0, 0);

[Severity: High]
Similarly, could this truncation cause memory corruption here?

If the firmware writes back the state using a truncated 32-bit physical
address, it may corrupt unintended memory regions if the original address
was above 4GB.

> +		/*
> +		 * If writeback fails, COMPLETE resumes with firmware's original
> +		 * a6/a7 rather than treating the shared buffer as committed.
> +		 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911121456.33223-1-zhangzhanpeng.jasper@bytedance.com?part=2