[PATCH] firmware: fw_base: add an acquire fence after the _boot_status wait loop

[Nickolai Zeldovich]

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

Non-boot harts exit _wait_for_boot_hart on a plain load of _boot_status
and go on to read platform.hart_index2id and other data the boot hart
relocated before publishing.  Nothing orders those loads after the
status load, so under RVWMO a hart can read the unrelocated pointer and
fault through it before mtvec is set.

Pair the boot hart's release fence with a "fence r, rw" after the loop.

Signed-off-by: Nickolai Zeldovich <nickolai.zeldovich at gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply at 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:

Non-boot harts leave _wait_for_boot_hart on a plain load of _boot_status
and immediately read data the boot hart produced before publishing.
Nothing on the waiting side orders that load before the later loads.
Under RVWMO a waiter can observe the "done" status and still read pre-
relocation values, including the unrelocated platform.hart_index2id
pointer, and fault through it before mtvec is set. One fence r, rw after
the loop closes it.

An execution that triggers it:

Setup: fw_dynamic loaded at an address different from its link address,
so the boot hart runs the self-relocation pass; eight harts; RVWMO
hardware (or any model that lets a plain load be satisfied early, e.g.
the Sail/MachCSL model used to find this).

1. Hart 1 reaches _wait_for_boot_hart while hart 0 is still relocating.
   Hart 1's loop body is ld t1, _boot_status; div; div; div; bne. Its
   load of _boot_status returns the pre-done value, it loops.
2. Hart 1's core speculatively issues the *next* iteration's loads
   early. RVWMO allows a later load to be performed before an earlier
   load to a different address when nothing orders them. In particular
   the loads that follow the loop exit, lwu platform.hart_count, ld
   platform.hart_index2id, may be satisfied now, from memory that hart 0
   has not yet rewritten: platform.hart_index2id still holds its link-
   time value 0x449f8.
3. Hart 0 finishes: it fixes up platform.hart_index2id to 0x800449f8 via
   the R_RISCV_RELATIVE pass, executes fence rw, rw, stores _boot_status
   = BOOT_STATUS_BOOT_HART_DONE.
4. Hart 1's loop load now returns DONE, the bne falls through. The
   already-performed load of platform.hart_index2id is not re-executed:
   s9 = 0x449f8.
5. Hart 1 executes lwu a5, 0(s9): a load from 0x449f8, which is outside
   every PMA region on the virt platform. The access faults.
6. Hart 1 has not yet executed _start_warm's csrw mtvec; mtvec holds its
   reset value. The trap vectors to an undefined address and the hart is
   lost. The system boots with 7 harts, or hangs in smp_init waiting for
   it.

The release on the boot hart (fence rw, rw before the status store) is
correct; the bug is the missing acquire on the waiting side. The fence
added here sits *after* the loop and runs once per hart. A 2019 attempt
placed a fence rw, rw inside the loop before the load (05602e2bf481) and
was reverted (69d794cbcbab) after a warm-reset regression on the HiFive
Unleashed whose root cause was never found; that fence did not provide
the needed ordering anyway.

Verification:

Proved: with the fence, the non-boot harts' path from _start to
_start_warm satisfies its contract against the Sail RISC-V model under
an RVWMO-faithful memory model. Without the fence the stale read is an
allowed execution and the contract is unprovable.

firmware/fw_base.S | 11 +++++++++++ 1 file changed, 11 insertions(+)

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

diff --git a/firmware/fw_base.S b/firmware/fw_base.S
index a76ddc1b..294787a2 100644
--- a/firmware/fw_base.S
+++ b/firmware/fw_base.S
@@ -407,6 +407,17 @@ _wait_for_boot_hart:
 	div	t2, t2, zero
 	div	t2, t2, zero
 	bne	t0, t1, _wait_for_boot_hart
+	/*
+	 * Acquire: order the _boot_status load above before every later
+	 * load (platform.hart_count, platform.hart_index2id, the scratch
+	 * space, ...).  The boot hart publishes with "fence rw, rw" before
+	 * its store to _boot_status, but without a matching acquire here
+	 * RVWMO lets a waiting hart that saw BOOT_STATUS_BOOT_HART_DONE
+	 * still read pre-relocation values, e.g. the unrelocated
+	 * platform.hart_index2id pointer, and fault through it before
+	 * mtvec is set.
+	 */
+	fence	r, rw
 
 _start_warm:
 	/* Reset all registers except ra, a0, a1, a2, a3 and a4 for non-boot HART */
-- 
2.55.0