Skip to contentSkip to conversation

RISC-V: KVM: Fix PMU event info array size overflow

PATCH RESEND In review Guidong Han 3 participants 3 messages Jul 30 – Aug 8

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH RESEND v2 1/1 RISC-V: KVM: Fix PMU event info array size overflow

SBI PMU EVENT_GET_INFO stores guest-controlled
num_events * sizeof(*einfo) in an int. On RV64, num_events = 0x10000001 makes 0x100000010 truncate to 16. KVM then allocates one entry but loops over the original num_events, causing out-of-bounds reads and writes. A nested guest triggered:

BUG: KASAN: slab-out-of-bounds in kvm_riscv_vcpu_pmu_event_info+0xa4/0x142
Read of size 4 at addr ff600000074d46b0 by task init/1
Call Trace:
[<ffffffff8006471c>] kvm_riscv_vcpu_pmu_event_info+0xa4/0x142
[<ffffffff800690c0>] kvm_sbi_ext_pmu_handler+0xca/0x268
[<ffffffff8006779e>] kvm_riscv_vcpu_sbi_ecall+0xec/0x1e6
[<ffffffff8006008c>] kvm_riscv_vcpu_exit+0x48c/0x540
[<ffffffff8005ea0a>] kvm_arch_vcpu_ioctl_run+0x37e/0xc80
Allocated by task 1:
 __kmalloc_noprof+0x19e/0x4b0
 kvm_riscv_vcpu_pmu_event_info+0x72/0x142
 kvm_sbi_ext_pmu_handler+0xca/0x268
 kvm_riscv_vcpu_sbi_ecall+0xec/0x1e6
 kvm_riscv_vcpu_exit+0x48c/0x540
 kvm_arch_vcpu_ioctl_run+0x37e/0xc80

The buggy address is located 0 bytes to the right of
allocated 16-byte region [ff600000074d46a0, ff600000074d46b0)

Store the shared-memory size in size_t and reject multiplication overflow. Allocate the guest-driven array with GFP_KERNEL_ACCOUNT so it is charged to kmemcg, and use __GFP_NOWARN to suppress allocation failure warnings. Use kvcalloc() to allow vmalloc fallback and an unsigned long loop index to match num_events.

Fixes e309fd113b9f ("RISC-V: KVM: Implement get event info function")

---
RESEND:
- Send v2 as a new top-level thread instead of replying to v1.

Changes in v2:
- Use an accounted, no-warning kvcalloc() allocation and kvfree(), as suggested by Sashiko.
- Naveed Khan reported the issue before v1. I missed his report at the time, so add Reported-by and Closes tags to credit him.

Link to v1: https://lore.kernel.org/kvm/[email protected]/

---
 arch/riscv/kvm/vcpu_pmu.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
arch/riscv/kvm/vcpu_pmu.c +9 -5
index bb46dcbfb24d..8e1f6abb84e2 100644
@@ -12,7 +12,9 @@
 #include <linux/err.h>
 #include <linux/kvm_host.h>
 #include <linux/nospec.h>
+#include <linux/overflow.h>
 #include <linux/perf/riscv_pmu.h>
+#include <linux/slab.h>
 #include <asm/csr.h>
 #include <asm/kvm_isa.h>
 #include <asm/kvm_vcpu_sbi.h>
@@ -479,13 +481,14 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
 				  unsigned long flags, struct kvm_vcpu_sbi_return *retdata)
 {
 	struct riscv_pmu_event_info *einfo = NULL;
-	int shmem_size = num_events * sizeof(*einfo);
+	size_t shmem_size;
 	gpa_t shmem;
 	u32 eidx, etype;
 	u64 econfig;
 	int ret;
 
-	if (flags != 0 || (saddr_low & (SZ_16 - 1) || num_events == 0)) {
+	if (flags != 0 || (saddr_low & (SZ_16 - 1)) || num_events == 0 ||
+	    check_mul_overflow(num_events, sizeof(*einfo), &shmem_size)) {
 		ret = SBI_ERR_INVALID_PARAM;
 		goto out;
 	}
@@ -500,7 +503,8 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
 		}
 	}
 
-	einfo = kzalloc(shmem_size, GFP_KERNEL);
+	einfo = kvcalloc(num_events, sizeof(*einfo),
+			 GFP_KERNEL_ACCOUNT | __GFP_NOWARN);
 	if (!einfo) {
 		ret = SBI_ERR_FAILURE;
 		goto out;
@@ -512,7 +516,7 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
 		goto free_mem;
 	}
 
-	for (int i = 0; i < num_events; i++) {
+	for (unsigned long i = 0; i < num_events; i++) {
 		eidx = einfo[i].event_idx;
 		etype = kvm_pmu_get_perf_event_type(eidx);
 		econfig = kvm_pmu_get_perf_event_config(eidx, einfo[i].event_data);
@@ -525,7 +529,7 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
 		ret = SBI_ERR_INVALID_ADDRESS;
 
 free_mem:
-	kfree(einfo);
+	kvfree(einfo);
 out:
 	retdata->err_val = ret;
 
Signature · 1 line
2.43.0
#1 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unbounded memory allocation and long execution loop due to unvalidated guest-controlled…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbounded memory allocation and long execution loop due to unvalidated guest-controlled num_events. --

--- Patch [1]: [PATCH v2 RESEND] RISC-V: KVM: Fix PMU event info array size overflow --- commit bdf470babff9670998ba5d0cae1674a0cc0b80b2
Author: Guidong Han <[email protected]>

RISC-V: KVM: Fix PMU event info array size overflow

This commit addresses an integer overflow in the SBI PMU event info array allocation. It switches the allocation to kvcalloc with GFP_KERNEL_ACCOUNT and uses size_t and unsigned long variables to match the guest-provided size without truncation.

Quoted 5 lines
arch/riscv/kvm/vcpu_pmu.c +0 -0
index 2025b664961c8..a7f948410d531 100644
@@ -479,13 +481,14 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low

[ ... ]

Quoted 6 lines
arch/riscv/kvm/vcpu_pmu.c +2 -1
-	if (flags != 0 || (saddr_low & (SZ_16 - 1) || num_events == 0)) {
+	if (flags != 0 || (saddr_low & (SZ_16 - 1)) || num_events == 0 ||
+	    check_mul_overflow(num_events, sizeof(*einfo), &shmem_size)) {
 		ret = SBI_ERR_INVALID_PARAM;
 		goto out;
 	}

[ ... ]

arch/riscv/kvm/vcpu_pmu.c int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low · line 507

Quoted 3 lines
arch/riscv/kvm/vcpu_pmu.c +2 -1
-	einfo = kzalloc(shmem_size, GFP_KERNEL);
+	einfo = kvcalloc(num_events, sizeof(*einfo),
+			 GFP_KERNEL_ACCOUNT | __GFP_NOWARN);

[Severity: High]
Can this introduce an unbounded memory allocation regression based on a guest-controlled num_events?

While check_mul_overflow() prevents integer overflow during the calculation, there doesn't appear to be a check capping num_events to a reasonable architectural maximum. A guest passing an extremely large value could cause kvcalloc() to attempt an allocation approaching the host's total RAM, which could lead to severe host memory thrashing or direct reclaim.

[ ... ]

arch/riscv/kvm/vcpu_pmu.c int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low · line 521

Quoted 4 lines
arch/riscv/kvm/vcpu_pmu.c +1 -1
-	for (int i = 0; i < num_events; i++) {
+	for (unsigned long i = 0; i < num_events; i++) {
 		eidx = einfo[i].event_idx;
 		etype = kvm_pmu_get_perf_event_type(eidx);

[Severity: High]
Could this loop iteration trigger a regression resulting in host soft lockups or RCU stalls?

If a massive allocation from the earlier kvcalloc() succeeds, this loop will iterate num_events times synchronously without yielding. This could allow an untrusted guest to monopolize the host CPU.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer Reviewed-by on v2 1/1
#2 lore raw
In reply to Guidong Han
Quoted 31 lines · Guidong Han

SBI PMU EVENT_GET_INFO stores guest-controlled
num_events * sizeof(*einfo) in an int. On RV64, num_events = 0x10000001 makes 0x100000010 truncate to 16. KVM then allocates one entry but loops over the original num_events, causing out-of-bounds reads and writes. A nested guest triggered:

BUG: KASAN: slab-out-of-bounds in kvm_riscv_vcpu_pmu_event_info+0xa4/0x142
Read of size 4 at addr ff600000074d46b0 by task init/1
Call Trace:
[<ffffffff8006471c>] kvm_riscv_vcpu_pmu_event_info+0xa4/0x142
[<ffffffff800690c0>] kvm_sbi_ext_pmu_handler+0xca/0x268
[<ffffffff8006779e>] kvm_riscv_vcpu_sbi_ecall+0xec/0x1e6
[<ffffffff8006008c>] kvm_riscv_vcpu_exit+0x48c/0x540
[<ffffffff8005ea0a>] kvm_arch_vcpu_ioctl_run+0x37e/0xc80
Allocated by task 1:
 __kmalloc_noprof+0x19e/0x4b0
 kvm_riscv_vcpu_pmu_event_info+0x72/0x142
 kvm_sbi_ext_pmu_handler+0xca/0x268
 kvm_riscv_vcpu_sbi_ecall+0xec/0x1e6
 kvm_riscv_vcpu_exit+0x48c/0x540
 kvm_arch_vcpu_ioctl_run+0x37e/0xc80

The buggy address is located 0 bytes to the right of
allocated 16-byte region [ff600000074d46a0, ff600000074d46b0)

Store the shared-memory size in size_t and reject multiplication overflow. Allocate the guest-driven array with GFP_KERNEL_ACCOUNT so it is charged to kmemcg, and use __GFP_NOWARN to suppress allocation failure warnings. Use kvcalloc() to allow vmalloc fallback and an unsigned long loop index to match num_events.

Quoted 5 lines
Fixes e309fd113b9f ("RISC-V: KVM: Implement get event info function")

LGTM.

Queued this patch for Linux-7.3

Thanks,
Anup

Quoted 76 lines

---
RESEND:
- Send v2 as a new top-level thread instead of replying to v1.

Changes in v2:
- Use an accounted, no-warning kvcalloc() allocation and kvfree(), as suggested by Sashiko.
- Naveed Khan reported the issue before v1. I missed his report at the time, so add Reported-by and Closes tags to credit him.

Link to v1: https://lore.kernel.org/kvm/[email protected]/

---
 arch/riscv/kvm/vcpu_pmu.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
arch/riscv/kvm/vcpu_pmu.c +9 -6
index bb46dcbfb24d..8e1f6abb84e2 100644
@@ -12,7 +12,9 @@
 #include <linux/err.h>
 #include <linux/kvm_host.h>
 #include <linux/nospec.h>
+#include <linux/overflow.h>
 #include <linux/perf/riscv_pmu.h>
+#include <linux/slab.h>
 #include <asm/csr.h>
 #include <asm/kvm_isa.h>
 #include <asm/kvm_vcpu_sbi.h>
@@ -479,13 +481,14 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
                                  unsigned long flags, struct kvm_vcpu_sbi_return *retdata)
 {
        struct riscv_pmu_event_info *einfo = NULL;
-       int shmem_size = num_events * sizeof(*einfo);
+       size_t shmem_size;
        gpa_t shmem;
        u32 eidx, etype;
        u64 econfig;
        int ret;
 
-       if (flags != 0 || (saddr_low & (SZ_16 - 1) || num_events == 0)) {
+       if (flags != 0 || (saddr_low & (SZ_16 - 1)) || num_events == 0 ||
+           check_mul_overflow(num_events, sizeof(*einfo), &shmem_size)) {
                ret = SBI_ERR_INVALID_PARAM;
                goto out;
        }
@@ -500,7 +503,8 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
                }
        }
 
-       einfo = kzalloc(shmem_size, GFP_KERNEL);
+       einfo = kvcalloc(num_events, sizeof(*einfo),
+                        GFP_KERNEL_ACCOUNT | __GFP_NOWARN);
        if (!einfo) {
                ret = SBI_ERR_FAILURE;
                goto out;
@@ -512,7 +516,7 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
                goto free_mem;
        }
 
-       for (int i = 0; i < num_events; i++) {
+       for (unsigned long i = 0; i < num_events; i++) {
                eidx = einfo[i].event_idx;
                etype = kvm_pmu_get_perf_event_type(eidx);
                econfig = kvm_pmu_get_perf_event_config(eidx, einfo[i].event_data);
@@ -525,7 +529,7 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
                ret = SBI_ERR_INVALID_ADDRESS;
 
 free_mem:
-       kfree(einfo);
+       kvfree(einfo);
 out:
        retdata->err_val = ret;
 
--

2.43.0