When a guest invokes SBI_EXT_PMU_COUNTER_FW_READ on a firmware counter
that has not been configured via SBI_EXT_PMU_COUNTER_CFG_MATCH, the
pmc->event_idx remains SBI_PMU_EVENT_IDX_INVALID (0xFFFFFFFF).
get_event_code() extracts the lower 16 bits, yielding 0xFFFF (65535),
which is then used to index into kvpmu->fw_event[]. Since fw_event is
only RISCV_KVM_MAX_FW_CTRS (32) entries, this triggers an
array-index-out-of-bounds:
UBSAN: array-index-out-of-bounds in arch/riscv/kvm/vcpu_pmu.c:255:37
index 65535 is out of range for type 'kvm_fw_event [32]'
Add a bounds check on fevent_code before accessing the fw_event array,
returning -EINVAL for invalid event codes.
Fixes: badc386869e2c ("RISC-V: KVM: Support firmware events")
Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
Signed-off-by: Jiakai Xu <jiakaiPeanut@gmail.com>
---
arch/riscv/kvm/vcpu_pmu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
index e873430e596b..c6d42459c2a1 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -252,6 +252,10 @@ static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
if (pmc->cinfo.type == SBI_PMU_CTR_TYPE_FW) {
fevent_code = get_event_code(pmc->event_idx);
+ if (fevent_code >= SBI_PMU_FW_MAX) {
+ pr_warn("Invalid firmware event code [%d] for counter [%ld]\n", fevent_code, cidx);
+ return -EINVAL;
+ }
pmc->counter_val = kvpmu->fw_event[fevent_code].value;
} else if (pmc->perf_event) {
pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
--
2.34.1On Fri, Mar 06, 2026 at 07:37:38AM +0000, Jiakai Xu wrote:
> When a guest invokes SBI_EXT_PMU_COUNTER_FW_READ on a firmware counter
> that has not been configured via SBI_EXT_PMU_COUNTER_CFG_MATCH, the
> pmc->event_idx remains SBI_PMU_EVENT_IDX_INVALID (0xFFFFFFFF).
> get_event_code() extracts the lower 16 bits, yielding 0xFFFF (65535),
> which is then used to index into kvpmu->fw_event[]. Since fw_event is
> only RISCV_KVM_MAX_FW_CTRS (32) entries, this triggers an
> array-index-out-of-bounds:
>
> UBSAN: array-index-out-of-bounds in arch/riscv/kvm/vcpu_pmu.c:255:37
> index 65535 is out of range for type 'kvm_fw_event [32]'
>
> Add a bounds check on fevent_code before accessing the fw_event array,
> returning -EINVAL for invalid event codes.
>
> Fixes: badc386869e2c ("RISC-V: KVM: Support firmware events")
> Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
> Signed-off-by: Jiakai Xu <jiakaiPeanut@gmail.com>
> ---
> arch/riscv/kvm/vcpu_pmu.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index e873430e596b..c6d42459c2a1 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -252,6 +252,10 @@ static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
>
> if (pmc->cinfo.type == SBI_PMU_CTR_TYPE_FW) {
> fevent_code = get_event_code(pmc->event_idx);
> + if (fevent_code >= SBI_PMU_FW_MAX) {
> + pr_warn("Invalid firmware event code [%d] for counter [%ld]\n", fevent_code, cidx);
We shouldn't add warnings for these types of things since a guest could
fill the host's log buffer. At most it could be a warn-once, but I think
we should just return the error as quickly as possible. It looks like
there are other pr_warns in the pmu emulation code that should probably
be audited to see if they should be removed or changed to warn-once as
well.
Thanks,
drew
> + return -EINVAL;
> + }
> pmc->counter_val = kvpmu->fw_event[fevent_code].value;
> } else if (pmc->perf_event) {
> pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
> --
> 2.34.1
>
>
> --
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
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
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
Hi drew,
Thanks for the clarification.
> 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.
I will submit the pr_warn cleanup as a separate patch series later.
> 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.
Agreed. In v2, I will update the selftests to include both positive tests
and negative tests.
> > 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.
>
Noted, I will fix it in the next version.
> > + read_fw_counter(i, ctrinfo);
> > }
> > }
> >
> > --
> > 2.34.1
> >
I'll send out v2 shortly.
Thanks,
Jiakai
© 2016 - 2026 Red Hat, Inc.