Re: [RFC 2/3] 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 2 potential issue(s) to consider:

New issues:
- [High] The patch introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability by reading `*pending` twice without `READ_ONCE()`, failing to fully prevent the undefined behavior it intends to fix.

Pre-existing issues:
- [High] The exact same `__ffs(0)` undefined behavior bug exists in the `booke.c` architecture variant but was omitted from this fix.
--

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

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

This patch adds a check to ensure the pending_exceptions bitmap is not empty
before calling __ffs() in the Book3S KVM PPC code. This prevents undefined
behavior that previously caused arbitrary interrupt delivery and guest hangs.

> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 2efbe05caed76..e4152e9a0d967 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -401,17 +401,19 @@ 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);
> -	while (priority < BOOK3S_IRQPRIO_MAX) {
> -		if (kvmppc_book3s_irqprio_deliver(vcpu, priority) &&
> -		    clear_irqprio(vcpu, priority)) {
> -			clear_bit(priority, &vcpu->arch.pending_exceptions);
> -			break;
> -		}
> +	if (*pending) {
> +		priority = __ffs(*pending);

[Severity: High]
Does this read *pending twice without READ_ONCE() and introduce a 
Time-of-Check to Time-of-Use (TOCTOU) vulnerability?

The KVM_INTERRUPT ioctl can be invoked concurrently by a host userspace
thread. It calls kvm_arch_vcpu_unlocked_ioctl(), which does not hold
vcpu->mutex, and proceeds to kvmppc_core_dequeue_external() where it
clears bits in vcpu->arch.pending_exceptions locklessly using the atomic
clear_bit().

Since the compiler can emit two separate memory loads for *pending, if the
last pending bit is cleared by a concurrent KVM_INTERRUPT ioctl between
the if (*pending) check and the __ffs(*pending) evaluation, could the
second load read 0 and still trigger the __ffs(0) undefined behavior this
patch intends to fix?


[Severity: High]
This isn't a bug introduced by this patch, but does the same __ffs(0)
undefined behavior still exist in the BookE architecture variant?

Looking at kvmppc_core_check_exceptions() in arch/powerpc/kvm/booke.c:

arch/powerpc/kvm/booke.c:kvmppc_core_check_exceptions() {
    ...
    priority = __ffs(*pending);
    while (priority < BOOKE_IRQPRIO_MAX) {
    ...
}

This is called unconditionally by kvmppc_core_prepare_to_enter() on every
VM entry:

arch/powerpc/kvm/booke.c:kvmppc_core_prepare_to_enter() {
    ...
    kvmppc_core_check_exceptions(vcpu);
    ...
}

Since there is no check against 0, it appears __ffs(0) is executed on every
guest entry when no exceptions are pending on BookE guests. Should a similar
fix be applied to booke.c as well?

> +		while (priority < BOOK3S_IRQPRIO_MAX) {
> +			if (kvmppc_book3s_irqprio_deliver(vcpu, priority) &&
> +			    clear_irqprio(vcpu, priority)) {
> +				clear_bit(priority, &vcpu->arch.pending_exceptions);
> +				break;
> +			}
>  
> -		priority = find_next_bit(pending,
> -					 BITS_PER_BYTE * sizeof(*pending),
> -					 priority + 1);
> +			priority = find_next_bit(pending,
> +						 BITS_PER_BYTE * sizeof(*pending),
> +						 priority + 1);
> +		}
>  	}
>  
>  	/* Tell the guest about our interrupt status */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789099725.git.ritesh.list@gmail.com?part=2