Re: [PATCH v2 RESEND] RISC-V: KVM: Fix PMU event info array size overflow

[sashiko-bot@kernel.org]

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 <2045gemini@gmail.com>

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.

Closes: https://lore.kernel.org/kvm/178345245327.72065.13249716450708539854@digiscrypt.com/

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 2025b664961c8..a7f948410d531 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -479,13 +481,14 @@ int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low
[ ... ]
> -	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;
>  	}
[ ... ]
> -	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.

[ ... ]
> -	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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730092533.1369531-1-2045gemini@gmail.com?part=1