Skip to content

Commit 4fc0f9e

Browse files
sean-jcgregkh
authored andcommitted
KVM: x86: Make x2APIC ID 100% readonly
commit 4b7c3f6 upstream. 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 a92e254 ("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 <[email protected]> Closes: https://lore.kernel.org/all/[email protected] Reported-by: Haoyu Wu <[email protected]> Closes: https://lore.kernel.org/all/[email protected] Reported-by: [email protected] Closes: https://lore.kernel.org/all/[email protected] Signed-off-by: Sean Christopherson <[email protected]> Message-ID: <[email protected]> Signed-off-by: Paolo Bonzini <[email protected]> Signed-off-by: James Houghton <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 8ea0e7b commit 4fc0f9e

File tree

1 file changed

+15
-7
lines changed

1 file changed

+15
-7
lines changed

arch/x86/kvm/lapic.c

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,8 @@ static void kvm_recalculate_logical_map(struct kvm_apic_map *new,
338338
* reversing the LDR calculation to get cluster of APICs, i.e. no
339339
* additional work is required.
340340
*/
341-
if (apic_x2apic_mode(apic)) {
342-
WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic)));
341+
if (apic_x2apic_mode(apic))
343342
return;
344-
}
345343

346344
if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr,
347345
&cluster, &mask))) {
@@ -2964,18 +2962,28 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
29642962
struct kvm_lapic_state *s, bool set)
29652963
{
29662964
if (apic_x2apic_mode(vcpu->arch.apic)) {
2965+
u32 x2apic_id = kvm_x2apic_id(vcpu->arch.apic);
29672966
u32 *id = (u32 *)(s->regs + APIC_ID);
29682967
u32 *ldr = (u32 *)(s->regs + APIC_LDR);
29692968
u64 icr;
29702969

29712970
if (vcpu->kvm->arch.x2apic_format) {
2972-
if (*id != vcpu->vcpu_id)
2971+
if (*id != x2apic_id)
29732972
return -EINVAL;
29742973
} else {
2974+
/*
2975+
* Ignore the userspace value when setting APIC state.
2976+
* KVM's model is that the x2APIC ID is readonly, e.g.
2977+
* KVM only supports delivering interrupts to KVM's
2978+
* version of the x2APIC ID. However, for backwards
2979+
* compatibility, don't reject attempts to set a
2980+
* mismatched ID for userspace that hasn't opted into
2981+
* x2apic_format.
2982+
*/
29752983
if (set)
2976-
*id >>= 24;
2984+
*id = x2apic_id;
29772985
else
2978-
*id <<= 24;
2986+
*id = x2apic_id << 24;
29792987
}
29802988

29812989
/*
@@ -2984,7 +2992,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
29842992
* split to ICR+ICR2 in userspace for backwards compatibility.
29852993
*/
29862994
if (set) {
2987-
*ldr = kvm_apic_calc_x2apic_ldr(*id);
2995+
*ldr = kvm_apic_calc_x2apic_ldr(x2apic_id);
29882996

29892997
icr = __kvm_lapic_get_reg(s->regs, APIC_ICR) |
29902998
(u64)__kvm_lapic_get_reg(s->regs, APIC_ICR2) << 32;

0 commit comments

Comments
 (0)