[PATCH] lib: sbi: Install the next-stage entry as a direct-mode xtvec

[PATCH] lib: sbi: Install the next-stage entry as a direct-mode xtvec

[Nickolai Zeldovich]

From: Nickolai Zeldovich <nickolai.zeldovich@gmail.com>

sbi_hart_switch_mode() writes next_addr unmasked into stvec (vstvec,
utvec).  Bits [1:0] of xtvec are the MODE field, so a 2-byte aligned
entry, legal with the C extension, selects a reserved mode: QEMU drops
the write, the Sail model keeps the old mode, and the next stage starts
with a trap vector that is not its entry.

Mask the mode bits and install the entry as a direct-mode vector; mepc
still receives the exact address.

Signed-off-by: Nickolai Zeldovich <nickolai.zeldovich@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
---
This bug was found in the process of formally verifying the correctness
of OpenSBI on top of the Sail RISC-V semantics using Lean.

Summary:

sbi_hart_switch_mode() writes the next stage's entry address to mepc
and, for an S-mode target, to stvec (vstvec for a virtualised target,
utvec for U-mode with N). mepc tolerates any instruction address: with
the C extension only bit 0 is cleared. stvec does not: bits [1:0] are
the MODE field, and the values 2 and 3 are reserved. An entry address
that is 2-byte but not 4-byte aligned, legal and common with the C
extension, has low bits 10 and asks for a reserved mode. The outcome is
implementation-defined, and in no implementation is it "stvec = entry".
Mask the mode bits so the entry is installed as a direct-mode vector.

An execution that triggers it:

Setup: a kernel built with the C extension whose secondary entry is
compressed-aligned. Current Linux is one: secondary_start_sbi is at
0xffffffff800000a6, physical 0x802000a6, so
__pa_symbol(secondary_start_sbi) ends in …a6, low bits 10.

1. The kernel's __cpu_up for CPU 3 calls sbi_hsm_hart_start(hartid = 3,
   saddr = 0x802000a6, opaque) (cpu_ops_sbi.c: sbi_cpu_start).
2. OpenSBI's sbi_hsm_hart_start() records next_addr = 0x802000a6,
   next_mode = PRV_S in hart 3's scratch and rings it. Hart 3 leaves
   sbi_hsm_hart_wait(), runs the warm init, and
   sbi_hsm_hart_start_finish() calls sbi_hart_switch_mode(hartid,
   opaque, 0x802000a6, PRV_S, false).
3. csr_write(CSR_MEPC, 0x802000a6): legal, mepc = 0x802000a6.
4. csr_write(CSR_STVEC, 0x802000a6): the MODE field is 2, reserved. What
   the hart does depends on the implementation:
- QEMU (target/riscv/tcg/csr.c: write_stvec) ignores the write and logs
     "CSR_STVEC: reserved mode not supported" under -d unimp. stvec
     keeps its previous value: whatever this hart had at reset or from a
     previous life. On virt after a cold reset that is 0.
- The Sail reference model (legalize_tvec with
     xtvec_mode_reserved_behavior = Xtvec_Ignore) keeps the previous
     MODE and takes the new base: stvec = 0x802000a4 | old_mode.
- Hardware may do either, or treat the value as direct mode; the spec
     leaves it open.
5. mret. Hart 3 is in S-mode at 0x802000a6 with sstatus.SIE = 0 and a
   trap vector that is not the entry: 0 (QEMU) or 0x802000a4 | mode
   (Sail).
6. Nothing bad happens *on this kernel*, because the first five
   instructions of secondary_start_sbi (csrw sie, 0; csrw sip, 0; …)
   cannot trap and the sixth is csrw stvec, .Lsecondary_park, which
   installs the kernel's own vector. The window is six instructions
   wide. What would go wrong, and where the bug bites, is any of:
- a next stage whose first instructions can fault (a load from a not-
     yet-mapped address, an illegal instruction from a misdetected
     extension, a misaligned access on a strict core) before it installs
     its own stvec. OpenSBI's intent, visible in the code, is that such
     a trap re-enters the next stage at its entry. Instead it vectors to
     0 (QEMU: sepc and scause are set, execution jumps to address 0,
     which is unmapped on virt, so a second fault happens with stvec
     still 0: an infinite trap loop, the hart is lost), or to entry - 2
     (Sail: into the middle of the preceding function's last compressed
     instruction);
- the Sail-model variant with the previous mode being *vectored* (mode
     1, left there by a previous OS or firmware stage): an interrupt in
     the window dispatches to entry - 2 + 4 * cause, into the kernel's
     code at an arbitrary offset;
- a previous stvec whose own MODE field is reserved (a value a prior
     stage wrote and that the hardware accepted literally): the Sail
     model flags the write as an internal error, i.e. the behaviour is
     undefined.

The fix writes next_addr & ~3UL to the xtvec register. For a 4-byte
aligned entry nothing changes. For a compressed-aligned entry the vector
is the 4-byte aligned address just below the entry, two bytes into the
preceding instruction stream; that is the best a direct-mode vector can
do, is well-defined on every implementation, and mepc, written unmasked,
still delivers control to the real entry. A next stage that relies on
the firmware-installed vector must have a 4-byte aligned entry; the
kernel does not, and installs its own.

Verification:

Proved: with the mask, the hand-off post-condition stvec = next_addr &
~3 holds against the Sail RISC-V model for any entry address. Without
the mask the post-condition stvec = next_addr is false for a 2-byte
aligned entry, and the outcome of the write depends on the previous
stvec, which nothing on the hart-start path constrains.

lib/sbi/sbi_hart.c | 18 +++++++++++++++--- 1 file changed, 15
insertions(+), 3 deletions(-)

FYI, the same change is also filed as GitHub pull request #431:
https://github.com/riscv-software-src/opensbi/pull/431

diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index bee88557..c0d64c83 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -751,6 +751,7 @@ sbi_hart_switch_mode(unsigned long arg0, unsigned long arg1,
 #else
 	unsigned long val;
 #endif
+	unsigned long tvec;
 
 	switch (next_mode) {
 	case PRV_M:
@@ -783,21 +784,32 @@ sbi_hart_switch_mode(unsigned long arg0, unsigned long arg1,
 	csr_write(CSR_MSTATUS, val);
 	csr_write(CSR_MEPC, next_addr);
 
+	/*
+	 * The xtvec registers encode the trap mode in bits [1:0]: 0 is
+	 * direct, 1 is vectored, 2 and 3 are reserved.  next_addr is an
+	 * instruction address and may be only 2-byte aligned when the C
+	 * extension is in use, in which case writing it unmasked asks
+	 * for a reserved mode and the result is implementation-defined
+	 * (the write is ignored, or the old mode is kept).  Install the
+	 * entry as a direct-mode vector instead.
+	 */
+	tvec = next_addr & ~0x3UL;
+
 	if (next_mode == PRV_S) {
 		if (next_virt) {
-			csr_write(CSR_VSTVEC, next_addr);
+			csr_write(CSR_VSTVEC, tvec);
 			csr_write(CSR_VSSCRATCH, 0);
 			csr_write(CSR_VSIE, 0);
 			csr_write(CSR_VSATP, 0);
 		} else {
-			csr_write(CSR_STVEC, next_addr);
+			csr_write(CSR_STVEC, tvec);
 			csr_write(CSR_SSCRATCH, 0);
 			csr_write(CSR_SIE, 0);
 			csr_write(CSR_SATP, 0);
 		}
 	} else if (next_mode == PRV_U) {
 		if (misa_extension('N')) {
-			csr_write(CSR_UTVEC, next_addr);
+			csr_write(CSR_UTVEC, tvec);
 			csr_write(CSR_USCRATCH, 0);
 			csr_write(CSR_UIE, 0);
 		}
-- 
2.55.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi: Install the next-stage entry as a direct-mode xtvec

[Chen Pei]

Hi Nickolai,

Thanks for tracking this down.

I ran into the same problem a while ago, but fixed it from the Linux
side by aligning secondary_start_sbi to 4 bytes:

https://lore.kernel.org/linux-riscv/20260413132009.133752-1-cp0613@linux.alibaba.com/

With the C extension, a 2-byte aligned start_addr is a perfectly
legal input to sbi_hsm_hart_start(), so OpenSBI should not turn it
into a reserved xtvec mode. I support this fix, and I see it as
complementary to my kernel-side change rather than an alternative to
it: the firmware fix covers every next stage, while an aligned entry
simply makes the mask a no-op.

Tested on QEMU riscv64 virt (-smp 4) with a kernel whose
secondary_start_sbi is 2-byte aligned: before the patch, "-d unimp"
logs three "CSR_STVEC: reserved mode not supported" messages; with
the patch, none, and all 4 CPUs come up.

> sbi_hart_switch_mode() writes next_addr unmasked into stvec (vstvec,
> utvec).  Bits [1:0] of xtvec are the MODE field, so a 2-byte aligned
> entry, legal with the C extension, selects a reserved mode: QEMU drops
> the write, the Sail model keeps the old mode, and the next stage starts
> with a trap vector that is not its entry.
>
> Mask the mode bits and install the entry as a direct-mode vector; mepc
> still receives the exact address.
>
> Signed-off-by: Nickolai Zeldovich <nickolai.zeldovich@gmail.com>

Tested-by: Chen Pei <cp0613@linux.alibaba.com>

Best regards,
Pei

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi