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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] riscv: premature restoration of CSR_SSTATUS re-enables interrupts leading to privilege escalation
- [High] riscv: truncation of 64-bit physical addresses on RV32 during SBI ecall
--

commit 491bc3336b6e02d6ec5b41c80039d8746df8e36e
Author: Clément Léger <cleger@rivosinc.com>

riscv: add support for SBI Supervisor Software Events extension

This commit introduces support for the SBI Supervisor Software Events
extension, allowing firmware to notify supervisor software of independent
events. It implements event registration, handler contexts with dedicated
stacks, and restoration of the interrupted supervisor state.

diff --git a/arch/riscv/kernel/sbi_sse.c b/arch/riscv/kernel/sbi_sse.c
new file mode 100644
index 0000000000000..7dd496e2bdb0e
--- /dev/null
+++ b/arch/riscv/kernel/sbi_sse.c
@@ -0,0 +1,246 @@
[ ... ]
> +void noinstr do_sse(struct sse_event_arch_data *arch_evt,
> +		    struct pt_regs *regs, unsigned long hstatus)
> +{
[ ... ]
> +	/* 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 this cast to unsigned long truncate the upper 32 bits of the physical 
address on RV32 systems?

On RV32 systems with 34-bit physical addressing (Sv32), phys_addr_t is 
64-bit. If the event data is allocated in memory residing above the 4GB 
boundary, this truncation could cause the firmware to write the interrupted 
registers to the wrong physical address in the lower 4GB, resulting in 
memory corruption.

> +	if (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, does this truncation happen identically on the writeback path?

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

diff --git a/arch/riscv/kernel/sbi_sse_entry.S b/arch/riscv/kernel/sbi_sse_entry.S
new file mode 100644
index 0000000000000..e0e8efba12ddb
--- /dev/null
+++ b/arch/riscv/kernel/sbi_sse_entry.S
@@ -0,0 +1,226 @@
[ ... ]
> +	csrw CSR_SSCRATCH, s4
> +	ALTERNATIVE("nop", "csrw hstatus, s5", 0, RISCV_ISA_EXT_H, 1)
> +	csrw CSR_STVEC, s3
> +
> +	REG_L a0, PT_STATUS(sp)
> +	REG_L a1, PT_EPC(sp)
> +	REG_L a2, PT_BADADDR(sp)
> +	REG_L a3, PT_CAUSE(sp)
> +	csrw CSR_SSTATUS, a0
> +	csrw CSR_EPC, a1
> +	csrw CSR_STVAL, a2
> +	csrw CSR_SCAUSE, a3

[Severity: Critical]
Does restoring the interrupted context's sstatus here prematurely re-enable 
interrupts before the handler completes natively via the SBI firmware?

If the interrupted context had interrupts enabled (SR_SIE bit set), writing 
it back immediately re-enables interrupts while still executing on the kernel's
SSE stack. 

If a pending interrupt fires during the window before the ecall, 
handle_exception reads the previously restored sscratch. If sscratch contains
a user task pointer, handle_exception could incorrectly swap the user's 
TASK_TI_USER_SP with the kernel SSE stack pointer, leaking the kernel stack
to userspace.

> +
> +	REG_L ra, PT_RA(sp)
> +	REG_L s0, PT_S0(sp)
[ ... ]

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