Re: [PATCH] RISC-V: KVM: Fix heap overflow in SBI PMU event info handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] An unbounded, guest-controlled `num_events` parameter translates into a large memory allocation, triggering a `WARN_ON_ONCE` in the host's page allocator.
--

--- Patch [1]: [PATCH] RISC-V: KVM: Fix heap overflow in SBI PMU event info handler ---
commit 59f34c484dfc616073369a4a2f4ea4eb32bc7ab5
Author: Naveed Khan <naveed@digiscrypt.com>

RISC-V: KVM: Fix heap overflow in SBI PMU event info handler

This commit fixes a heap out-of-bounds read and write by using size_mul()
to calculate the shared memory size, saturating to SIZE_MAX on overflow
so the allocation fails safely.

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index bb46dcbfb24da..ff940fa74b55d 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c

[ ... ]

> @@ -479,7 +480,7 @@ 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 = size_mul(num_events, sizeof(*einfo));

[Severity: High]
This is a pre-existing issue, but does this allocation still allow a guest
to trigger a host warning?

Because num_events comes directly from the guest without an upper bound, a
malicious guest can provide an extremely large value. While size_mul() now
safely saturates to SIZE_MAX and prevents the loop mismatch, this massive
size is subsequently passed to kzalloc():

arch/riscv/kvm/vcpu_pmu.c:kvm_riscv_vcpu_pmu_event_info() {
    ...
    einfo = kzalloc(shmem_size, GFP_KERNEL);
    ...
}

Since GFP_KERNEL lacks the __GFP_NOWARN flag, passing a size that evaluates
to an order larger than MAX_PAGE_ORDER will trigger a WARN_ON_ONCE in the
host's page allocator.

If panic_on_warn is enabled on the host, this results in a guest-triggerable
host denial of service. Should this allocation include __GFP_NOWARN, or
should num_events be explicitly bounded before the allocation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178345245327.72065.13249716450708539854@digiscrypt.com?part=1