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

[Anup Patel]

On Mon, Oct 12, 2026 at 3:51 AM Nickolai Zeldovich
<nickolai at csail.mit.edu> wrote:
>
> 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>

Use Assisted-by instead of Co-Authored-By for LLMs. I will
update at the time of merging.

Reviewed-by: Anup Patel <anup at brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
> 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
>