Re: [PATCH 1/2] KVM: nVMX: don't check PIR.ON when processing nested posted interrupts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: spurious vector 255 injected due to TOCTOU race in PIR update
- [High] KVM: nVMX: missing compiler barrier after pi_clear_on() leads to lost L2 interrupts
--

commit 3c2cccabb927f653f71a7a79b1d1600d4fb5f728
Author: Maxim Levitsky <mlevitsk@redhat.com>

    KVM: nVMX: don't check PIR.ON when processing nested posted interrupts

This commit modifies nested posted interrupt processing to no longer require
the PIR.ON bit to be set. It achieves this by replacing the conditional
pi_test_and_clear_on() check with a direct pi_clear_on() call.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ddf6df7bee93b..11362aad2bc8a 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4033,8 +4033,11 @@ static int vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
>  
>  	vmx->nested.pi_pending = false;
>  
> -	if (!pi_test_and_clear_on(vmx->nested.pi_desc))
> -		return 0;
> +	/*
> +	 * Don't test the value of PID.ON.
> +	 * It is valid to trigger a posted interrupt without setting this bit
> +	 */
> +	pi_clear_on(vmx->nested.pi_desc);

[Severity: High]
Does this change introduce a missing compiler barrier that could result
in lost L2 interrupts?

Replacing pi_test_and_clear_on() (which uses test_and_clear_bit(),
providing a full memory and compiler barrier) with pi_clear_on() (which
uses clear_bit()) removes the implicit compiler barrier. On x86,
clear_bit() uses inline assembly without a memory clobber, providing
no compiler barrier.

Without an explicit smp_mb__after_atomic(), could the compiler reorder the
earlier vmx->nested.pi_pending = false store to after the ON bit is
cleared?

If reordered, the CPU could execute the PIR read inside
pi_find_highest_vector() before committing the pi_pending = false store.
If a concurrent software IPI triggers
vmx_deliver_nested_posted_interrupt() on another CPU during this window:

- It will set the new PIR bit.
- It will set the ON bit.
- It will assert vmx->nested.pi_pending = true.

The delayed pi_pending = false store would then silently overwrite the
concurrent true signal. Since the PIR was already read prior to the new
bit being set, KVM would miss and permanently lose the interrupt.

>  
>  	max_irr = pi_find_highest_vector(vmx->nested.pi_desc);
>  	if (max_irr > 0) {

[ ... ]

>  		__kvm_apic_update_irr(vmx->nested.pi_desc->pir,
>  			vapic_page, &max_irr);
>  		status = vmcs_read16(GUEST_INTR_STATUS);
>  		if ((u8)max_irr > ((u8)status & 0xff)) {
>  			status &= ~0xff;
>  			status |= (u8)max_irr;
>  			vmcs_write16(GUEST_INTR_STATUS, status);
>  		}

[Severity: High]
Could this execution path erroneously inject a spurious interrupt vector
255 into the L2 guest?

By removing the pi_test_and_clear_on() gate, the code now unconditionally
falls through to __kvm_apic_update_irr() whenever max_irr > 0 and
pi_pending is true.

If an L1 guest concurrently modifies its PIR/IRR memory structures during
L2 posted interrupt processing, clearing the PIR and IRR in guest memory,
__kvm_apic_update_irr() can update max_irr to -1.

When max_irr is -1, the code subsequently casts it to u8:
((u8)(-1) == 255)

Since 255 is typically greater than the current RVI, KVM would write 255
to GUEST_INTR_STATUS. Can this cause L2 guest crashes or undefined
behavior by injecting vector 255?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009205036.672523-2-mlevitsk@redhat.com?part=1