linux-kernel - Re: [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrD9HHaMBqNGEaaW@google.com>
Date: Mon, 5 Aug 2024 09:26:04 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Michal Luczaj <mhal@...x.co>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Haoyu Wu <haoyuwu254@...il.com>, 
	syzbot+545f1326f405db4e1c3e@...kaller.appspotmail.com
Subject: Re: [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly

On Sun, Aug 04, 2024, Michal Luczaj wrote:
> On 8/2/24 22:29, Sean Christopherson wrote:
> > [...]
> > Making the x2APIC ID fully readonly fixes a WARN in KVM's optimized map
> > calculation, which expects the LDR to align with the x2APIC ID.
> > 
> >   WARNING: CPU: 2 PID: 958 at arch/x86/kvm/lapic.c:331 kvm_recalculate_apic_map+0x609/0xa00 [kvm]
> >   CPU: 2 PID: 958 Comm: recalc_apic_map Not tainted 6.4.0-rc3-vanilla+ #35
> >   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-1-1 04/01/2014
> >   RIP: 0010:kvm_recalculate_apic_map+0x609/0xa00 [kvm]
> >   Call Trace:
> >    <TASK>
> >    kvm_apic_set_state+0x1cf/0x5b0 [kvm]
> >    kvm_arch_vcpu_ioctl+0x1806/0x2100 [kvm]
> >    kvm_vcpu_ioctl+0x663/0x8a0 [kvm]
> >    __x64_sys_ioctl+0xb8/0xf0
> >    do_syscall_64+0x56/0x80
> >    entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >   RIP: 0033:0x7fade8b9dd6f
> 
> Isn't this WARN_ON_ONCE() inherently racy, though? With your patch applied,
> it can still be hit by juggling the APIC modes.

Doh, right, the logic is unfortunately cross-vCPU.  The sanity check could be
conditioned on the APIC belonging to the running/loaded vCPU, but I'm leaning
towards deleting it entirely.  Though it did detect the KVM_SET_LAPIC backdoor...

Anyone have a preference, or better idea?

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index a7172ba59ad2..67a0c116ebc0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -352,7 +352,8 @@ static void kvm_recalculate_logical_map(struct kvm_apic_map *new,
         * additional work is required.
         */
        if (apic_x2apic_mode(apic)) {
-               WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic)));
+               WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic)) &&
+                            vcpu == kvm_get_running_vcpu());
                return;
        }