[PATCH] lib: sbi_pmu: Do not return uninitialized stack data on FW read errors

[Nickolai Zeldovich]

sbi_ecall_pmu_handler() passes the uninitialized local temp to
sbi_pmu_ctr_fw_read() and copies it into out->value unconditionally.
sbi_pmu_ctr_fw_read() only writes *cval on success, so every error
return of SBI_EXT_PMU_COUNTER_FW_READ (e.g. a hardware or inactive
counter index) hands an M-mode stack word to the supervisor in a1.
SBI_EXT_PMU_COUNTER_FW_READ_HI does the same with the upper half of
temp on RV32.

Initialize temp to zero so that the error paths return zero in
sbiret.value; the success paths are unchanged.

Fixes: 1664d0efce0b ("lib: sbi_pmu: Replace sbi_pmu_ctr_read() with sbi_pmu_ctr_fw_read()")
Fixes: 51951d9e9af8 ("lib: sbi_pmu: Implement sbi_pmu_counter_fw_read_hi")
Signed-off-by: Nickolai Zeldovich <nickolai at csail.mit.edu>
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.

Triggering execution: any S-mode caller,
  a7 = 0x504D55 (PMU), a6 = 5 (COUNTER_FW_READ), a0 = 0 (mcycle).
sbi_pmu_ctr_fw_read() returns SBI_EINVAL at its first range check
without writing *cval, the handler copies the never-written temp into
out->value, and sbi_ecall_handler() returns it in a1 next to
a0 = SBI_ERR_INVALID_PARAM.  temp has its address taken, so it lives
in a stack slot of the M-mode trap stack and a1 receives whatever the
last M-mode code left there.  The same holds for a6 = 6 on RV32
(upper half); the RV64 arm of fid 6 already returns 0.

The same verification pass found that SBI_PMU_CFG_FLAG_SKIP_MATCH lets
a supervisor pair a firmware counter with a hardware event (sbi_panic()
via csr_write_num(0xb20)) or a hardware counter with a firmware event
(an out-of-bounds fw_counters_data[] store); that is fixed by the
pending "lib: sbi: reject PMU skip-match counter class mismatches"
from Pengpeng Hou, so it is not repeated here.

Testing:
- make PLATFORM=generic LLVM=1 PLATFORM_RISCV_XLEN=64: builds, no
  warnings.  PLATFORM_RISCV_XLEN=32 (rv32imac_zicsr_zifencei/ilp32):
  sbi_ecall_pmu.c compiles without warnings; the 32-bit build as a
  whole stops on an unrelated, pre-existing clang
  -Wsometimes-uninitialized error in lib/sbi/sbi_trap_ldst.c that
  origin/master has too.
- A userspace model of the handler (fid 5/6, sbi_pmu_ctr_fw_read()
  stubbed with the real "write *cval only on success" behaviour) shows
  the stale word in a1 on the error path before the patch and 0 after.
- Not run on QEMU (none available here).  Suggested check: from an
  S-mode test payload (or kvm-unit-tests' SBI PMU test), call
  sbi_ecall(0x504D55, 5, 0, ...) right after a successful fw read of
  a firmware counter configured with COUNTER_CFG_MATCH; before the
  patch a1 holds the previous call's counter value (whatever is in
  the stack slot), after it a1 = 0.

 lib/sbi/sbi_ecall_pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
index 41bb1624..0762cce5 100644
--- a/lib/sbi/sbi_ecall_pmu.c
+++ b/lib/sbi/sbi_ecall_pmu.c
@@ -22,7 +22,7 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
 				 struct sbi_ecall_return *out)
 {
 	int ret = 0;
-	uint64_t temp;
+	uint64_t temp = 0;
 
 	switch (funcid) {
 	case SBI_EXT_PMU_NUM_COUNTERS:

base-commit: 6ad246a1494807ee91e36ffcf60e4c30bb309d45
-- 
2.53.0