| Message ID | 20230526235048.2842761-4-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Out-of-bounds access in kvm_recalculate_phys_map() | expand |
On Fri, May 26, 2023, Sean Christopherson wrote: > From: Michal Luczaj <mhal@rbox.co> > > Keep switching between LAPIC_MODE_X2APIC and LAPIC_MODE_DISABLED during > APIC map construction to hunt for TOCTOU bugs in KVM. KVM's optimized map > recalc makes multiple passes over the list of vCPUs, and the calculations > ignore vCPU's whose APIC is hardware-disabled, i.e. there's a window where > toggling LAPIC_MODE_DISABLED is quite interesting. > > Signed-off-by: Michal Luczaj <mhal@rbox.co> > Co-developed-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > tools/testing/selftests/kvm/Makefile | 1 + > .../kvm/x86_64/recalc_apic_map_race.c | 76 +++++++++++++++++++ > 2 files changed, 77 insertions(+) > create mode 100644 tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c Since there's another bug+test related to kvm_recalculate_apic_map()[*], I think it makes sense to name this recalc_apic_map_test, and then fold the LDR test into this one. The LDR test is tiny enough that I don't think it's worth a separate binary, even though I generally prefer to keep the selftests small.
On Thu, Jun 01, 2023, Sean Christopherson wrote: > On Fri, May 26, 2023, Sean Christopherson wrote: > > From: Michal Luczaj <mhal@rbox.co> > > > > Keep switching between LAPIC_MODE_X2APIC and LAPIC_MODE_DISABLED during > > APIC map construction to hunt for TOCTOU bugs in KVM. KVM's optimized map > > recalc makes multiple passes over the list of vCPUs, and the calculations > > ignore vCPU's whose APIC is hardware-disabled, i.e. there's a window where > > toggling LAPIC_MODE_DISABLED is quite interesting. > > > > Signed-off-by: Michal Luczaj <mhal@rbox.co> > > Co-developed-by: Sean Christopherson <seanjc@google.com> > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > tools/testing/selftests/kvm/Makefile | 1 + > > .../kvm/x86_64/recalc_apic_map_race.c | 76 +++++++++++++++++++ > > 2 files changed, 77 insertions(+) > > create mode 100644 tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c > > Since there's another bug+test related to kvm_recalculate_apic_map()[*], I think > it makes sense to name this recalc_apic_map_test, and then fold the LDR test into > this one. The LDR test is tiny enough that I don't think it's worth a separate > binary, even though I generally prefer to keep the selftests small. Actually, the x2APIC test is a better fit in xapic_state_test. I'll probably still rename this to match (almost) every other selftest name.
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 7a5ff646e7e7..c9b8f16fb23f 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -116,6 +116,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests 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_race 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_apic_map_race.c b/tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c new file mode 100644 index 000000000000..09516548c11a --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/recalc_apic_map_race.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * recalc_apic_map_race + * + * Test for a race condition in kvm_recalculate_apic_map(). + */ + +#include <sys/ioctl.h> +#include <pthread.h> +#include <time.h> + +#include "processor.h" +#include "test_util.h" +#include "kvm_util.h" +#include "apic.h" + +#define TIMEOUT 5 /* seconds */ + +#define LAPIC_DISABLED 0 +#define LAPIC_X2APIC (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE) +#define MAX_XAPIC_ID 0xff + +static void *race(void *arg) +{ + struct kvm_lapic_state lapic = {}; + struct kvm_vcpu *vcpu = arg; + + while (1) { + /* Trigger kvm_recalculate_apic_map(). */ + vcpu_ioctl(vcpu, KVM_SET_LAPIC, &lapic); + pthread_testcancel(); + } + + return NULL; +} + +int main(void) +{ + struct kvm_vcpu *vcpus[KVM_MAX_VCPUS]; + struct kvm_vcpu *vcpuN; + struct kvm_vm *vm; + pthread_t thread; + time_t t; + int i; + + kvm_static_assert(KVM_MAX_VCPUS > MAX_XAPIC_ID); + + /* + * Creating the max number of vCPUs supported by selftests so that KVM + * has decent amount of work to do when recalculating the map, i.e. to + * make the problematic window large enough to hit. + */ + vm = vm_create_with_vcpus(KVM_MAX_VCPUS, NULL, vcpus); + + /* + * Enable x2APIC on all vCPUs so that KVM doesn't bail from the recalc + * due to vCPUs having aliased xAPIC IDs (truncated to 8 bits). + */ + for (i = 0; i < KVM_MAX_VCPUS; i++) + vcpu_set_msr(vcpus[i], MSR_IA32_APICBASE, LAPIC_X2APIC); + + ASSERT_EQ(pthread_create(&thread, NULL, race, vcpus[0]), 0); + + vcpuN = vcpus[KVM_MAX_VCPUS - 1]; + for (t = time(NULL) + TIMEOUT; time(NULL) < t;) { + vcpu_set_msr(vcpuN, MSR_IA32_APICBASE, LAPIC_X2APIC); + vcpu_set_msr(vcpuN, MSR_IA32_APICBASE, LAPIC_DISABLED); + } + + ASSERT_EQ(pthread_cancel(thread), 0); + ASSERT_EQ(pthread_join(thread, NULL), 0); + + kvm_vm_release(vm); + + return 0; +}