Re: [PATCH 6.6] KVM: x86: Make x2APIC ID 100% readonly

[Sean Christopherson <seanjc@google.com>]

On Sat, Dec 28, 2024, Sasha Levin wrote:
> On Thu, Dec 26, 2024 at 11:38:47AM +0800, Gavin Guo wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > [ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
> > 
> > Ignore the userspace provided x2APIC ID when fixing up APIC state for
> > KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM.  Commit
> > a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID
> > register"), which added the fixup, didn't intend to allow userspace to
> > modify the x2APIC ID.  In fact, that commit is when KVM first started
> > treating the x2APIC ID as readonly, apparently to fix some race:
> > 
> > static inline u32 kvm_apic_id(struct kvm_lapic *apic)
> > {
> > -       return (kvm_lapic_get_reg(apic, APIC_ID) >> 24) & 0xff;
> > +       /* To avoid a race between apic_base and following APIC_ID update when
> > +        * switching to x2apic_mode, the x2apic mode returns initial x2apic id.
> > +        */
> > +       if (apic_x2apic_mode(apic))
> > +               return apic->vcpu->vcpu_id;
> > +
> > +       return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> > }
> > 
> > Furthermore, KVM doesn't support delivering interrupts to vCPUs with a
> > modified x2APIC ID, but KVM *does* return the modified value on a guest
> > RDMSR and for KVM_GET_LAPIC.  I.e. no remotely sane setup can actually
> > work with a modified x2APIC ID.
> > 
> > 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
> > 
> > Unfortunately, the WARN can still trigger for other CPUs than the current
> > one by racing against KVM_SET_LAPIC, so remove it completely.
> > 
> > Reported-by: Michal Luczaj <mhal@rbox.co>
> > Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@rbox.co
> > Reported-by: Haoyu Wu <haoyuwu254@gmail.com>
> > Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@gmail.com
> > Reported-by: syzbot+545f1326f405db4e1c3e@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@google.com
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > Message-ID: <20240802202941.344889-2-seanjc@google.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Gavin Guo <gavinguo@igalia.com>
> 
> As this one isn't tagged for stable, the KVM maintainers should ack the
> backport before we take it.

What's the motivation for applying this to 6.6?  AFAIK, there's no real world use
case that benefits from the patch, the fix is purely to plug a hole where fuzzers,
e.g. syzkaller, can trip a WARN.

That said, this is essentially a prerequisite for "KVM: x86: Re-split x2APIC ICR
into ICR+ICR2 for AMD (x2AVIC)"[*], and it's relatively low risk, so I'm not
opposed to landing it in 6.6.

[*] https://lore.kernel.org/all/2024100123-unreached-enrage-2cb1@gregkh