Re: [PATCHv2 2/7] KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: PPC: Book3S: out-of-bounds read in kvmppc_core_prepare_to_enter()
--

commit 05f5afe597ebfa07475ccb012aebebd6487a463b
Author: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap

This commit replaces __ffs(*pending) with find_first_bit() in
kvmppc_core_prepare_to_enter() so that an empty pending_exceptions bitmap
does not invoke undefined behavior and deliver a bogus interrupt to the
guest.

> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 2efbe05caed76..38816578d002e 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -401,7 +401,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>  	if (vcpu->arch.pending_exceptions)
>  		printk(KERN_EMERG "KVM: Check pending: %lx\n", vcpu->arch.pending_exceptions);
>  #endif
> -	priority = __ffs(*pending);
> +	priority = find_first_bit(pending, BITS_PER_LONG * sizeof(*pending));

[Severity: High]
Does passing BITS_PER_LONG * sizeof(*pending) as the bit size to
find_first_bit() in kvmppc_core_prepare_to_enter() (in
arch/powerpc/kvm/book3s.c) cause an out-of-bounds read past
vcpu->arch.pending_exceptions when no exceptions are pending?

vcpu->arch.pending_exceptions is a single unsigned long in struct
kvm_vcpu_arch (arch/powerpc/include/asm/kvm_host.h):

	unsigned long pending_exceptions;

Because BITS_PER_LONG is already the number of bits in an unsigned long and
sizeof(*pending) is the byte size, BITS_PER_LONG * sizeof(*pending)
evaluates to 512 bits (8 unsigned longs) on 64-bit PowerPC or 128 bits (4
unsigned longs) on 32-bit PowerPC.

Since that size exceeds BITS_PER_LONG, small_const_nbits(size) in
find_first_bit() (include/linux/find.h) evaluates to false and calls
_find_first_bit() in lib/find_bit.c, which iterates across multiple words
and reads pending[1..7] (or pending[1..3]) out of bounds into adjacent
fields of struct kvm_vcpu_arch whenever vcpu->arch.pending_exceptions is 0.

Note that the find_next_bit() call later in the same loop uses
BITS_PER_BYTE:

		priority = find_next_bit(pending,
					 BITS_PER_BYTE * sizeof(*pending),
					 priority + 1);

Should find_first_bit() use BITS_PER_BYTE * sizeof(*pending) (or simply
BITS_PER_LONG) here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b985cc12b0ed62599815ddf87e612bc8b6b2813c.1793543627.git.ritesh.list@gmail.com?part=1