* [PATCH 0/2] KVM: x86: Disallow changing x2APIC ID via userspace @ 2024-08-02 20:29 Sean Christopherson 2024-08-02 20:29 ` [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly Sean Christopherson 2024-08-02 20:29 ` [PATCH 2/2] KVM: selftests: Add a testcase to verify x2APIC is fully readonly Sean Christopherson 0 siblings, 2 replies; 6+ messages in thread From: Sean Christopherson @ 2024-08-02 20:29 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: kvm, linux-kernel, Michal Luczaj, Haoyu Wu, syzbot+545f1326f405db4e1c3e Silently ignore userspace attempts to change the x2APIC ID via KVM_SET_LAPIC. I've been hunting for this series since January[*], and *finally* stumbled across it while tidying up my (too) many git trees. [*] https://lore.kernel.org/all/ZbPkHvuJv0EdJhVN@google.com Michal Luczaj (1): KVM: selftests: Add a testcase to verify x2APIC is fully readonly Sean Christopherson (1): KVM: x86: Make x2APIC ID 100% readonly arch/x86/kvm/lapic.c | 18 +++++++++--- .../selftests/kvm/x86_64/xapic_state_test.c | 28 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) base-commit: 332d2c1d713e232e163386c35a3ba0c1b90df83f -- 2.46.0.rc2.264.g509ed76dc8-goog ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly 2024-08-02 20:29 [PATCH 0/2] KVM: x86: Disallow changing x2APIC ID via userspace Sean Christopherson @ 2024-08-02 20:29 ` Sean Christopherson 2024-08-04 20:30 ` Michal Luczaj 2024-08-02 20:29 ` [PATCH 2/2] KVM: selftests: Add a testcase to verify x2APIC is fully readonly Sean Christopherson 1 sibling, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2024-08-02 20:29 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: kvm, linux-kernel, Michal Luczaj, Haoyu Wu, syzbot+545f1326f405db4e1c3e 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 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> --- arch/x86/kvm/lapic.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index a7172ba59ad2..c6a59871acb3 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2966,18 +2966,28 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s, bool set) { if (apic_x2apic_mode(vcpu->arch.apic)) { + u32 x2apic_id = kvm_x2apic_id(vcpu->arch.apic); u32 *id = (u32 *)(s->regs + APIC_ID); u32 *ldr = (u32 *)(s->regs + APIC_LDR); u64 icr; if (vcpu->kvm->arch.x2apic_format) { - if (*id != vcpu->vcpu_id) + if (*id != x2apic_id) return -EINVAL; } else { + /* + * Ignore the userspace value when setting APIC state. + * KVM's model is that the x2APIC ID is readonly, e.g. + * KVM only supports delivering interrupts to KVM's + * version of the x2APIC ID. However, for backwards + * compatibility, don't reject attempts to set a + * mismatched ID for userspace that hasn't opted into + * x2apic_format. + */ if (set) - *id >>= 24; + *id = x2apic_id; else - *id <<= 24; + *id = x2apic_id << 24; } /* @@ -2986,7 +2996,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, * split to ICR+ICR2 in userspace for backwards compatibility. */ if (set) { - *ldr = kvm_apic_calc_x2apic_ldr(*id); + *ldr = kvm_apic_calc_x2apic_ldr(x2apic_id); icr = __kvm_lapic_get_reg(s->regs, APIC_ICR) | (u64)__kvm_lapic_get_reg(s->regs, APIC_ICR2) << 32; -- 2.46.0.rc2.264.g509ed76dc8-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly 2024-08-02 20:29 ` [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly Sean Christopherson @ 2024-08-04 20:30 ` Michal Luczaj 2024-08-05 16:26 ` Sean Christopherson 0 siblings, 1 reply; 6+ messages in thread From: Michal Luczaj @ 2024-08-04 20:30 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: kvm, linux-kernel, Haoyu Wu, syzbot+545f1326f405db4e1c3e 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. [ 53.882945] WARNING: CPU: 13 PID: 1181 at arch/x86/kvm/lapic.c:355 kvm_recalculate_apic_map+0x335/0x650 [kvm] [ 53.883007] CPU: 13 UID: 1000 PID: 1181 Comm: recalc_logical_ Not tainted 6.11.0-rc1nokasan+ #18 [ 53.883009] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014 [ 53.883010] RIP: 0010:kvm_recalculate_apic_map+0x335/0x650 [kvm] [ 53.883057] Call Trace: [ 53.883058] <TASK> [ 53.883169] kvm_apic_set_state+0x105/0x3d0 [kvm] [ 53.883201] kvm_arch_vcpu_ioctl+0xf09/0x19c0 [kvm] [ 53.883285] kvm_vcpu_ioctl+0x6cc/0x920 [kvm] [ 53.883310] __x64_sys_ioctl+0x90/0xd0 [ 53.883313] do_syscall_64+0x93/0x180 [ 53.883623] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 53.883625] RIP: 0033:0x7fd90fee0d2d diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 48d32c5aa3eb..3344f1478230 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -129,6 +129,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/amx_test TEST_GEN_PROGS_x86_64 += x86_64/max_vcpuid_cap_test TEST_GEN_PROGS_x86_64 += x86_64/triple_fault_event_test TEST_GEN_PROGS_x86_64 += x86_64/recalc_apic_map_test +TEST_GEN_PROGS_x86_64 += x86_64/recalc_logical_map_warn TEST_GEN_PROGS_x86_64 += access_tracking_perf_test TEST_GEN_PROGS_x86_64 += demand_paging_test TEST_GEN_PROGS_x86_64 += dirty_log_test diff --git a/tools/testing/selftests/kvm/x86_64/recalc_logical_map_warn.c b/tools/testing/selftests/kvm/x86_64/recalc_logical_map_warn.c new file mode 100644 index 000000000000..ad3ae0433230 --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/recalc_logical_map_warn.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic))) + * in arch/x86/kvm/lapic.c:kvm_recalculate_logical_map() + */ + +#include <pthread.h> + +#include "processor.h" +#include "kvm_util.h" +#include "apic.h" + +#define LAPIC_XAPIC MSR_IA32_APICBASE_ENABLE +#define LAPIC_X2APIC (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) + +static void *race(void *arg) +{ + struct kvm_lapic_state state = {}; + struct kvm_vcpu *vcpu0 = arg; + + /* Trigger kvm_recalculate_apic_map(). */ + for (;;) + __vcpu_ioctl(vcpu0, KVM_SET_LAPIC, &state); + + return NULL; +} + +int main(void) +{ + struct kvm_vcpu *vcpus[2]; + struct kvm_vm *vm; + pthread_t thread; + + vm = vm_create_with_vcpus(2, NULL, vcpus); + + vcpu_set_msr(vcpus[1], MSR_IA32_APICBASE, LAPIC_X2APIC); + vcpu_set_msr(vcpus[1], APIC_BASE_MSR + (APIC_SPIV >> 4), APIC_SPIV_APIC_ENABLED); + + TEST_ASSERT_EQ(pthread_create(&thread, NULL, race, vcpus[0]), 0); + + for (;;) { + _vcpu_set_msr(vcpus[1], MSR_IA32_APICBASE, LAPIC_XAPIC); + _vcpu_set_msr(vcpus[1], MSR_IA32_APICBASE, LAPIC_X2APIC); + } + + kvm_vm_free(vm); + + return 0; +} ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly 2024-08-04 20:30 ` Michal Luczaj @ 2024-08-05 16:26 ` Sean Christopherson 2024-08-13 16:01 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2024-08-05 16:26 UTC (permalink / raw) To: Michal Luczaj Cc: Paolo Bonzini, kvm, linux-kernel, Haoyu Wu, syzbot+545f1326f405db4e1c3e 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; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly 2024-08-05 16:26 ` Sean Christopherson @ 2024-08-13 16:01 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2024-08-13 16:01 UTC (permalink / raw) To: Sean Christopherson, Michal Luczaj Cc: kvm, linux-kernel, Haoyu Wu, syzbot+545f1326f405db4e1c3e On 8/5/24 18:26, Sean Christopherson wrote: > 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... Yeah, let's drop it since we do have a test (in userspace). Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: selftests: Add a testcase to verify x2APIC is fully readonly 2024-08-02 20:29 [PATCH 0/2] KVM: x86: Disallow changing x2APIC ID via userspace Sean Christopherson 2024-08-02 20:29 ` [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly Sean Christopherson @ 2024-08-02 20:29 ` Sean Christopherson 1 sibling, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2024-08-02 20:29 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: kvm, linux-kernel, Michal Luczaj, Haoyu Wu, syzbot+545f1326f405db4e1c3e From: Michal Luczaj <mhal@rbox.co> Add a test to verify that userspace can't change a vCPU's x2APIC ID by abusing KVM_SET_LAPIC. KVM models the x2APIC ID (and x2APIC LDR) as readonly, and silently ignores userspace attempts to change the x2APIC ID for backwards compatibility. Signed-off-by: Michal Luczaj <mhal@rbox.co> [sean: write changelog, add to existing test] Signed-off-by: Sean Christopherson <seanjc@google.com> --- .../selftests/kvm/x86_64/xapic_state_test.c | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c index 69849acd95b0..618cd2442390 100644 --- a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c +++ b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c @@ -184,6 +184,33 @@ static void test_apic_id(void) kvm_vm_free(vm); } +static void test_x2apic_id(void) +{ + struct kvm_lapic_state lapic = {}; + struct kvm_vcpu *vcpu; + struct kvm_vm *vm; + int i; + + vm = vm_create_with_one_vcpu(&vcpu, NULL); + vcpu_set_msr(vcpu, MSR_IA32_APICBASE, MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE); + + /* + * Try stuffing a modified x2APIC ID, KVM should ignore the value and + * always return the vCPU's default/readonly x2APIC ID. + */ + for (i = 0; i <= 0xff; i++) { + *(u32 *)(lapic.regs + APIC_ID) = i << 24; + *(u32 *)(lapic.regs + APIC_SPIV) = APIC_SPIV_APIC_ENABLED; + vcpu_ioctl(vcpu, KVM_SET_LAPIC, &lapic); + + vcpu_ioctl(vcpu, KVM_GET_LAPIC, &lapic); + TEST_ASSERT(*((u32 *)&lapic.regs[APIC_ID]) == vcpu->id << 24, + "x2APIC ID should be fully readonly"); + } + + kvm_vm_free(vm); +} + int main(int argc, char *argv[]) { struct xapic_vcpu x = { @@ -211,4 +238,5 @@ int main(int argc, char *argv[]) kvm_vm_free(vm); test_apic_id(); + test_x2apic_id(); } -- 2.46.0.rc2.264.g509ed76dc8-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-08-13 16:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-02 20:29 [PATCH 0/2] KVM: x86: Disallow changing x2APIC ID via userspace Sean Christopherson 2024-08-02 20:29 ` [PATCH 1/2] KVM: x86: Make x2APIC ID 100% readonly Sean Christopherson 2024-08-04 20:30 ` Michal Luczaj 2024-08-05 16:26 ` Sean Christopherson 2024-08-13 16:01 ` Paolo Bonzini 2024-08-02 20:29 ` [PATCH 2/2] KVM: selftests: Add a testcase to verify x2APIC is fully readonly Sean Christopherson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.