[PATCH] lib: sbi_pmu: unmap shared memory on the early exit in event_get_info

[Sebastian Alba Vives]

sbi_pmu_event_get_info() maps the caller's shared memory into M-mode with
sbi_hart_protection_temp_map_range(). The must-be-zero check added inside the
event loop returns straight to the supervisor without reaching the matching
temp_unmap_range() at the end of the function, so the mapping is left behind.

This has a security impact beyond leaking a mapping. Under Smepmp the
temporary mapping is a single reserved PMP entry, SBI_SMEPMP_RESV_ENTRY.
sbi_hart_smepmp_temp_map_range() refuses to reprogram that entry while it is
already enabled:

	if (sbi_hart_is_pmp_enabled(SBI_SMEPMP_RESV_ENTRY))
		return SBI_ENOSPC;

and no caller in the tree checks that return value. sbi_pmu.c, sbi_sse.c,
sbi_mpxy.c, sbi_dbtr.c and sbi_ecall_dbcn.c all discard it. So once the entry
is left behind, every later shared memory ecall on that hart runs with PMP 0
still covering the previous region, and M-mode faults the first time it
dereferences the new buffer.

A supervisor triggers this with one ecall carrying a reserved bit in the first
event_idx. The next SBI_EXT_DBCN_CONSOLE_WRITE then takes an M-mode load
access fault on the console buffer and the firmware stops making forward
progress, taking DBCN, SSE, MPXY and DBTR down with it on that hart. It also
breaks the Smepmp invariant that M-mode does not retain standing access to
supervisor memory.

Confirmed on qemu virt with a bare metal S-mode payload, QEMU 8.2.2:

  build                 -cpu                protection   result
  master 6ad246a1       rv64                pmp          completes
  master 6ad246a1       rv64,smepmp=true    epmp         firmware dies
  master + this patch   rv64,smepmp=true    epmp         completes

The middle row leaves this as the last entry in the QEMU exception trace,
where the epc is inside nputs() in M-mode and the tval is the supervisor
console buffer:

  cause:0000000000000005, epc:0x000000008000204c,
  tval:0x0000000080500000, desc=fault_load

Without Smepmp the map and unmap pair compiles down to nothing, which is why
the first row survives.

This only affects master. v1.9 and earlier have the same map and unmap pair
but no return between them, so no released version needs a backport.

Route the early exit through the existing unmap.

Fixes: d97cfb33ae1a ("lib: sbi_pmu: Return invalid param error for reserved event_idx bits")
Signed-off-by: Sebastian Alba Vives <sebasjosue84 at gmail.com>
---

A note that came out of the same audit but does not belong in the commit
message, since it is a design question rather than part of this fix.

Every call site of sbi_hart_protection_temp_map_range() in the tree discards
the return value. There are eleven of them, in sbi_pmu.c, sbi_dbtr.c,
sbi_mpxy.c and sbi_ecall_dbcn.c. The function can return SBI_ENOSPC when the
reserved entry is already in use, and when that happens the caller goes on to
dereference the buffer with no mapping covering it, which is how the bug above
turns into a dead hart rather than an error code.

I checked the rest of the acquire and release pairs on the ecall paths while I
was there. sbi_dbtr_update_trig(), sbi_dbtr_install_trig(), the mpxy attribute
handlers and the SSE event get and put pairs are all balanced on every exit,
including the ones inside loops, so event_get_info() is the only unbalanced
path I found. The discarded return values are what makes that single slip
fatal instead of visible, so it may be worth deciding whether the callers
should check, or whether the helper should fail loudly on its own.

I am happy to send a follow up for whichever shape you prefer.

 lib/sbi/sbi_pmu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index e79f6b60..3cc1fc4b 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -1075,7 +1075,7 @@ int sbi_pmu_event_get_info(unsigned long shmem_phys_lo, unsigned long shmem_phys
 			   unsigned long num_events, unsigned long flags)
 {
 	unsigned long shmem_size = num_events * sizeof(struct sbi_pmu_event_info);
-	int i, j, event_type;
+	int i, j, event_type, ret = 0;
 	struct sbi_pmu_event_info *einfo;
 	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
 	uint32_t event_idx;
@@ -1112,8 +1112,10 @@ int sbi_pmu_event_get_info(unsigned long shmem_phys_lo, unsigned long shmem_phys
 	for (i = 0; i < num_events; i++) {
 		event_idx = einfo[i].event_idx;
 		/* Any must-be-zero event_idx bits set should return INVALID_PARAM per-spec */
-		if (event_idx & SBI_PMU_EVENT_IDX_MBZ_MASK)
-			return SBI_ERR_INVALID_PARAM;
+		if (event_idx & SBI_PMU_EVENT_IDX_MBZ_MASK) {
+			ret = SBI_ERR_INVALID_PARAM;
+			goto out_unmap;
+		}
 		event_type = pmu_event_validate(phs, event_idx, einfo[i].event_data);
 		if (event_type < 0) {
 			einfo[i].output = 0;
@@ -1154,9 +1156,10 @@ int sbi_pmu_event_get_info(unsigned long shmem_phys_lo, unsigned long shmem_phys
 		}
 	}
 
+out_unmap:
 	sbi_hart_protection_temp_unmap_range(shmem_phys_lo, shmem_size);
 
-	return 0;
+	return ret;
 }
 
 static void pmu_reset_event_map(struct sbi_pmu_hart_state *phs)
-- 
2.43.0