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

[Andrew Jones]

On Sat, Mar 07, 2026 at 02:35:37AM +0000, Jiakai Xu wrote:
> 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?

Any changes that come out of the pr_warn audit will result in a separate
patch or patches. That work can be done completely separately and submit
as a separate series. Or, if you do it right now, you could append those
patches to this series. Either way works for me.

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

That's good and we should do that, but we should also do negative testing.
So there should be a test case where we try to read a counter without
configuring it and ensure everything fails gracefully.

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

Put () around the & operator. checkpatch should have pointed that out.

> +				read_fw_counter(i, ctrinfo);
>  		}
>  	}
>  
> -- 
> 2.34.1
>

Thanks,
drew