Re: [PATCH 1/2] RISC-V: KVM: Fix array out-of-bounds in pmu_ctr_read()

[Jiakai Xu]

Hi Andrew,

Thanks for the review and the suggestions.

I agree with your feedback. In v2, I will merge the fixes for both
pmu_ctr_read() and pmu_fw_ctr_read_hi() into a single commit and remove
the pr_warn, simply returning -EINVAL instead.

I have two questions regarding the next version:

1. Regarding other pr_warns in PMU emulation code:
You mentioned that other pr_warns in the code might need auditing. Should
I address those in this patchset (e.g., converting them to pr_warn_once
or removing them), or is it better to keep this series focused strictly
on the out-of-bounds fix and handle the logging cleanup in a separate
patchset later?

2. Selftests update:
I noticed that applying this fix causes a regression in
selftests/kvm/sbi_pmu_test. The test_pmu_basic_sanity function currently
attempts to read a firmware counter without configuring it via
SBI_EXT_PMU_COUNTER_CFG_MATCH first.

Previously, this triggered the out-of-bounds access (likely reading
garbage), but with this fix, the kernel correctly returns
SBI_ERR_INVALID_PARAM, causing the test to fail.

To fix this, I plan to include a second patch in v2 that updates the
selftest to properly configure the counter before reading. Here is my
draft for the fix. Does this approach look reasonable to you?

Thanks,
Jiakai

---
 .../testing/selftests/kvm/include/riscv/sbi.h | 28 +++++++++++++++++++
 .../selftests/kvm/riscv/sbi_pmu_test.c        |  9 +++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/riscv/sbi.h b/tools/testing/selftests/kvm/include/riscv/sbi.h
index 046b432ae896..8c172422f386 100644
--- a/tools/testing/selftests/kvm/include/riscv/sbi.h
+++ b/tools/testing/selftests/kvm/include/riscv/sbi.h
@@ -97,6 +97,34 @@ enum sbi_pmu_hw_generic_events_t {
 	SBI_PMU_HW_GENERAL_MAX,
 };
 
+enum sbi_pmu_fw_generic_events_t {
+	SBI_PMU_FW_MISALIGNED_LOAD	= 0,
+	SBI_PMU_FW_MISALIGNED_STORE	= 1,
+	SBI_PMU_FW_ACCESS_LOAD		= 2,
+	SBI_PMU_FW_ACCESS_STORE		= 3,
+	SBI_PMU_FW_ILLEGAL_INSN		= 4,
+	SBI_PMU_FW_SET_TIMER		= 5,
+	SBI_PMU_FW_IPI_SENT		= 6,
+	SBI_PMU_FW_IPI_RCVD		= 7,
+	SBI_PMU_FW_FENCE_I_SENT		= 8,
+	SBI_PMU_FW_FENCE_I_RCVD		= 9,
+	SBI_PMU_FW_SFENCE_VMA_SENT	= 10,
+	SBI_PMU_FW_SFENCE_VMA_RCVD	= 11,
+	SBI_PMU_FW_SFENCE_VMA_ASID_SENT	= 12,
+	SBI_PMU_FW_SFENCE_VMA_ASID_RCVD	= 13,
+
+	SBI_PMU_FW_HFENCE_GVMA_SENT	= 14,
+	SBI_PMU_FW_HFENCE_GVMA_RCVD	= 15,
+	SBI_PMU_FW_HFENCE_GVMA_VMID_SENT = 16,
+	SBI_PMU_FW_HFENCE_GVMA_VMID_RCVD = 17,
+
+	SBI_PMU_FW_HFENCE_VVMA_SENT	= 18,
+	SBI_PMU_FW_HFENCE_VVMA_RCVD	= 19,
+	SBI_PMU_FW_HFENCE_VVMA_ASID_SENT = 20,
+	SBI_PMU_FW_HFENCE_VVMA_ASID_RCVD = 21,
+	SBI_PMU_FW_MAX,
+};
+
 /* SBI PMU counter types */
 enum sbi_pmu_ctr_type {
 	SBI_PMU_CTR_TYPE_HW = 0x0,
diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
index 924a335d2262..0d6ba3563561 100644
--- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
+++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
@@ -461,7 +461,14 @@ static void test_pmu_basic_sanity(void)
 			pmu_csr_read_num(ctrinfo.csr);
 			GUEST_ASSERT(illegal_handler_invoked);
 		} else if (ctrinfo.type == SBI_PMU_CTR_TYPE_FW) {
-			read_fw_counter(i, ctrinfo);
+			/*
+			 * Try to configure with a common firmware event.
+			 * If configuration succeeds, verify we can read it.
+			 */
+			ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH,
+			        i, 1, 0, SBI_PMU_FW_ACCESS_LOAD, 0, 0);
+			if (ret.error == 0 && ret.value < RISCV_MAX_PMU_COUNTERS && BIT(ret.value) & counter_mask_available)
+				read_fw_counter(i, ctrinfo);
 		}
 	}
 
-- 
2.34.1