Re: [PATCH 6.6.y 1/2] KVM: x86: Make x2APIC ID 100% readonly

[Sean Christopherson <seanjc@google.com>]

On Wed, Feb 05, 2025, James Houghton wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> 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>
> (cherry picked from commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071)

FWIW, for upstream LTS backports, the upstream commit information is usually place
at the top, before the original commit's changelog begins, and the blurb explicitly
calls out that it's an upstream commit.  I personally like this style:

  [ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]

as I find it easy to visually parse/separate from the original changelog.

> Signed-off-by: James Houghton <jthoughton@google.com>
> ---

Acked-by: Sean Christopherson <seanjc@google.com>