Re: [PATCH] KVM: Fix LDR inconsistency warning caused by APIC_ID format error

[Sean Christopherson <seanjc@google.com>]

On Sat, Jan 27, 2024, Haoyu Wu wrote:
> Syzkaller detected a warning in the kvm_recalculate_logical_map()
> function. This function employs VCPU_ID as the current x2APIC_ID
> following the apic_x2apic_mode() check. However, the LDR value,
> as computed using the current x2APIC_ID,  fails to align with the LDR
> value that is actually set.
> 
> Syzkaller scenario:
> 1) Set up VCPU's
> 2) Set the APIC_BASE to 0xd00
> 3) Set the APIC status for a specific state
> 
> The issue arises within kvm_apic_state_fixup, a function responsible
> for adjusting and correcting the APIC state. Initially, it verifies
> whether the current vcpu operates in x2APIC mode by examining the
> vcpu's mode. Subsequently, the function evaluates
> vcpu->kvm->arch.x2apic_format to ascertain if the preceding kvm version
> supports x2APIC mode. In cases where kvm is compatible with x2APIC mode,
> the function compares APIC_ID and VCPU_ID for equality. If they are not
> equal, it processes APIC_ID according to the set value. The error
> manifests when vcpu->kvm->arch.x2apic_format is false; under these
> circumstances, kvm_apic_state_fixup converts APIC_ID to the xAPIC format
> and invokes kvm_apic_calc_x2apic_ldr to compute the LDR. This leads to by
> passing consistency checks between VCPU_ID and APIC_ID and results in
> calling incorrect functions for LDR calculation.

Please just provide the syzkaller reproducer.

> Obviously, the crux of the issue hinges on the transition of the APIC
> state and the associated operations for transitioning APIC_ID. In the
> current kernel design, APIC_ID defaults to VCPU_ID in x2APIC mode, a
> specification not required in xAPIC mode. kvm_apic_state_fixup initiates
> by assessing the current status of both VCPU and KVM to identify their
> respective APIC modes. However, subsequent evaluations focus solely on
> the APIC mode of VCPU. To address this, a feasible minor modification
> involves advancing the comparison between APIC_ID and VCPU_ID,
> positioning it prior to the evaluation of vcpu→kvm→arch.x2apic_format.
>
> Signed-off-by: Haoyu Wu <haoyuwu254@gmail.com>
> ---
>  arch/x86/kvm/lapic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 3242f3da2..16c97d57d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2933,16 +2933,16 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
>  		u32 *ldr = (u32 *)(s->regs + APIC_LDR);
>  		u64 icr;
>  
> -		if (vcpu->kvm->arch.x2apic_format) {
> -			if (*id != vcpu->vcpu_id)
> -				return -EINVAL;
> -		} else {
> +		if (*id != vcpu->vcpu_id)
> +			return -EINVAL;

This will break userspace.  As shown below, if userspace is using the legacy
format, the incoming ID will be vcpu_id << 24.

This is a known issue[*], and apparently I had/have a patch?  I'll try to dredge
that up.  I suspect what I intended to was this, but I haven't yet found a branch
or stash, or anything else that captured what I intended to do.  *sigh*

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3242f3da2457..d25e31c04fbd 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2933,16 +2933,16 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
                u32 *ldr = (u32 *)(s->regs + APIC_LDR);
                u64 icr;
 
-               if (vcpu->kvm->arch.x2apic_format) {
-                       if (*id != vcpu->vcpu_id)
-                               return -EINVAL;
-               } else {
+               if (!vcpu->kvm->arch.x2apic_format) {
                        if (set)
                                *id >>= 24;
                        else
                                *id <<= 24;
                }
 
+               if (*id != vcpu->vcpu_id)
+                       return -EINVAL;
+
                /*
                 * In x2APIC mode, the LDR is fixed and based on the id.  And
                 * ICR is internally a single 64-bit register, but needs to be

[*] https://lore.kernel.org/all/ZHk3TGyB2Vze4+Ou@google.com

> +		if (!vcpu->kvm->arch.x2apic_format) {
>  			if (set)
>  				*id >>= 24;
>  			else
>  				*id <<= 24;
>  		}
>  
> +

Spurious whitespace.

>  		/*
>  		 * In x2APIC mode, the LDR is fixed and based on the id.  And
>  		 * ICR is internally a single 64-bit register, but needs to be
> -- 
> 2.34.1
>