Re: [PATCH v12 02/11] 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:
- [High] riscv: sbi_sse: field-spanning memcpy() on regs->a6 in do_sse()
- [High] riscv: sse: avoid clobbering tp with NULL in handle_sse() during __switch_to()
--

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

riscv: add support for SBI Supervisor Software Events extension

This commit adds architectural support for the RISC-V SBI Supervisor
Software Events (SSE) extension, including dedicated per-event stacks,
shadow call stacks, and low-level entry and exit assembly handling.

Link: https://lore.kernel.org/r/as79MHtNYd-VbUug@hu-himchau-blr.qualcomm.com/

> diff --git a/arch/riscv/kernel/sbi_sse.c b/arch/riscv/kernel/sbi_sse.c
> new file mode 100644
> index 0000000000000..032fcc07abcf7
> --- /dev/null
> +++ b/arch/riscv/kernel/sbi_sse.c
> @@ -0,0 +1,251 @@

[ ... ]

> +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);
> +	if (sret.error) {
> +		pr_warn("Failed to read interrupted registers for event %x: %ld\n",
> +			arch_evt->evt_id, sret.error);
> +		/* Let the client quiesce its source without using incomplete regs. */
> +		sse_handle_event(arch_evt, NULL);
> +		goto out;
> +	}
> +
> +	memcpy(&regs->a6, arch_evt->interrupted,
> +	       sizeof(*arch_evt->interrupted));
> +
> +	/* Make the interrupted frame visible while clients handle this event. */
> +	previous = this_cpu_read(riscv_sse_interrupted_context);
> +	this_cpu_write(riscv_sse_interrupted_context, &context);
> +	sse_handle_event(arch_evt, regs);
> +	this_cpu_write(riscv_sse_interrupted_context, previous);
> +
> +	if (memcmp(&regs->a6, arch_evt->interrupted,
> +		   sizeof(*arch_evt->interrupted))) {
> +		memcpy(arch_evt->interrupted, &regs->a6,
> +		       sizeof(*arch_evt->interrupted));

[Severity: High]
Does passing &regs->a6 to memcpy() and memcmp() with
sizeof(*arch_evt->interrupted) trigger a field-spanning overflow under
CONFIG_FORTIFY_SOURCE=y?

In struct pt_regs (arch/riscv/include/asm/ptrace.h), a6 and a7 are separate
unsigned long members:

	struct_group(a_regs,
		unsigned long a0;
		...
		unsigned long a6;
		unsigned long a7;
	);

Because &regs->a6 points to a single unsigned long field while
sizeof(*arch_evt->interrupted) spans two unsigned longs (a6 and a7),
fortify_memcpy_chk() sees __builtin_object_size(&regs->a6, 1) as smaller than
the copy size. This triggers a compile-time __write_overflow_field() error
when CONFIG_FORTIFY_SOURCE=y and CONFIG_WERROR=y are enabled (as well as
__read_overflow2_field() under W=1), or a runtime fortify_warn_once() inside
NMI context when an SSE event is delivered.

Could do_sse() read and write regs->a6 and regs->a7 directly (or group them
with struct_group()) instead of using memcpy() and memcmp() across the field
boundary?

[ ... ]

> diff --git a/arch/riscv/kernel/sbi_sse_entry.S b/arch/riscv/kernel/sbi_sse_entry.S
> new file mode 100644
> index 0000000000000..1e38fbb1c2a42
> --- /dev/null
> +++ b/arch/riscv/kernel/sbi_sse_entry.S
> @@ -0,0 +1,237 @@

[ ... ]

> +	asm_per_cpu_with_cpu t2 __sbi_sse_entry_task t1 t3
> +	REG_L tp, 0(t2)
> +	bnez tp, .Lcurrent_task_found
> +
> +	/* __switch_to() marks its transition window with a NULL entry task. */
> +	REG_L tp, PT_TP(sp)
> +
> +.Lcurrent_task_found:
> +	/* Nested exceptions temporarily replace these with the SSE stack. */
> +	REG_L s6, TASK_TI_KERNEL_SP(tp)

[Severity: High]
Can a higher-priority nested SSE event observe a NULL tp here if the first
event interrupted __switch_to()?

In __switch_to() (arch/riscv/kernel/entry.S), __sbi_sse_entry_task is cleared
to zero during the task switch window, relying on the interrupted tp register
remaining valid:

	asm_per_cpu t0, __sbi_sse_entry_task, t1
	REG_S zero, 0(t0)

When a lower-priority SSE event arrives during this window, handle_sse saves
the valid tp into PT_TP(sp) on its stack and then executes:

	REG_L tp, 0(t2)
	bnez tp, .Lcurrent_task_found
	REG_L tp, PT_TP(sp)

Because REG_L tp, 0(t2) loads NULL directly into tp before checking for zero,
both __sbi_sse_entry_task and the live tp register are simultaneously NULL
between those two loads. If a higher-priority SSE event preempts handle_sse
in that window, the nested handle_sse saves tp (now NULL) into its own
PT_TP(sp), loads NULL from __sbi_sse_entry_task, reloads NULL from PT_TP(sp),
and dereferences NULL at REG_L s6, TASK_TI_KERNEL_SP(tp).

Would loading __sbi_sse_entry_task into a temporary register first and only
moving it into tp when non-zero avoid clobbering the live tp register?

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