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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> + 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
>
