[PATCH 2/2] lib: sbi_pmu: reject out-of-bounds counter index in pmu_ctr_write_hw()

[Sebastian Alba Vives]

pmu_ctr_write_hw() writes CSR_MCYCLE + cidx with no check on cidx,
while its sibling pmu_ctr_start_hw() guards the same index space:

	if (cidx >= num_hw_ctrs || cidx == 1)
		return SBI_EINVAL;

A cidx at or above num_hw_ctrs addresses an mhpmcounter the platform
need not implement, and cidx == 1 addresses 0xb01, which is not
assigned. Either one traps in M-mode instead of returning an error.

Give the function the same guard and a return value. The guard is
copied from pmu_ctr_start_hw() rather than limited to the upper
bound, so that the two functions accept exactly the same index space.

Only the call site in sbi_pmu_ctr_cfg_match() propagates the error.
The two call sites inside pmu_ctr_start_hw() run after the identical
guard in that function, so they cannot fail and are left unchanged.

Fixes: 13d40f21d588 ("lib: sbi: Add PMU support")
Signed-off-by: Sebastian Alba Vives <sebasjosue84 at gmail.com>
---

A note on reachability, so the Fixes tag is not read as more than it
is.

Since 9f37e25f ("lib: sbi: reject PMU skip-match counter class
mismatches") the SKIP_MATCH path rejects a hardware event on a
firmware counter. Index 1 cannot reach this call site either:
active_events[1] is initialised to SBI_PMU_EVENT_IDX_INVALID and
pmu_ctr_find_hw() masks out the fixed counters with
~SBI_PMU_FIXED_CTR_MASK. I could not find a route that gets a bad
index into pmu_ctr_write_hw() on current master, so this is hardening
of a latent defect, not a live one.

I included cidx == 1 to match pmu_ctr_start_hw() exactly, which is
slightly more than the out-of-bounds check you asked for. If you would
rather the two functions guard different index spaces, I will resend
with only the upper bound.

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

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 62d5e206..74aa2fe0 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -389,8 +389,12 @@ static int pmu_ctr_enable_irq_hw(int ctr_idx)
 	return 0;
 }
 
-static void pmu_ctr_write_hw(uint32_t cidx, uint64_t ival)
+static int pmu_ctr_write_hw(uint32_t cidx, uint64_t ival)
 {
+	/* Make sure the counter index lies within the range and is not TM bit */
+	if (cidx >= num_hw_ctrs || cidx == 1)
+		return SBI_EINVAL;
+
 #if __riscv_xlen == 32
 	csr_write_num(CSR_MCYCLE + cidx, 0);
 	csr_write_num(CSR_MCYCLE + cidx, ival & 0xFFFFFFFF);
@@ -398,6 +402,8 @@ static void pmu_ctr_write_hw(uint32_t cidx, uint64_t ival)
 #else
 	csr_write_num(CSR_MCYCLE + cidx, ival);
 #endif
+
+	return 0;
 }
 
 static int pmu_ctr_start_hw(uint32_t cidx, uint64_t ival, bool ival_update)
@@ -980,8 +986,11 @@ skip_match:
 	    event_type == SBI_PMU_EVENT_TYPE_HW_CACHE ||
 	    event_type == SBI_PMU_EVENT_TYPE_HW_RAW ||
 	    event_type == SBI_PMU_EVENT_TYPE_HW_RAW_V2) {
-		if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
-			pmu_ctr_write_hw(ctr_idx, 0);
+		if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE) {
+			ret = pmu_ctr_write_hw(ctr_idx, 0);
+			if (ret)
+				return ret;
+		}
 		if (flags & SBI_PMU_CFG_FLAG_AUTO_START) {
 			ret = pmu_ctr_start_hw(ctr_idx, 0, false);
 			if (ret && ret != SBI_EALREADY_STARTED)
-- 
2.43.0