From 3bcbc20942db5d738221cca31a928efc09827069 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 21 Jul 2023 15:33:52 -0700 Subject: selftests/rseq: Play nice with binaries statically linked against glibc 2.35+ To allow running rseq and KVM's rseq selftests as statically linked binaries, initialize the various "trampoline" pointers to point directly at the expect glibc symbols, and skip the dlysm() lookups if the rseq size is non-zero, i.e. the binary is statically linked *and* the libc registered its own rseq. Define weak versions of the symbols so as not to break linking against libc versions that don't support rseq in any capacity. The KVM selftests in particular are often statically linked so that they can be run on targets with very limited runtime environments, i.e. test machines. Fixes: 233e667e1ae3 ("selftests/rseq: Uplift rseq selftests for compatibility with glibc-2.35") Cc: Aaron Lewis Cc: kvm@vger.kernel.org Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson Message-Id: <20230721223352.2333911-1-seanjc@google.com> Signed-off-by: Paolo Bonzini --- tools/testing/selftests/rseq/rseq.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c index 4e4aa006004c..a723da253244 100644 --- a/tools/testing/selftests/rseq/rseq.c +++ b/tools/testing/selftests/rseq/rseq.c @@ -34,9 +34,17 @@ #include "../kselftest.h" #include "rseq.h" -static const ptrdiff_t *libc_rseq_offset_p; -static const unsigned int *libc_rseq_size_p; -static const unsigned int *libc_rseq_flags_p; +/* + * Define weak versions to play nice with binaries that are statically linked + * against a libc that doesn't support registering its own rseq. + */ +__weak ptrdiff_t __rseq_offset; +__weak unsigned int __rseq_size; +__weak unsigned int __rseq_flags; + +static const ptrdiff_t *libc_rseq_offset_p = &__rseq_offset; +static const unsigned int *libc_rseq_size_p = &__rseq_size; +static const unsigned int *libc_rseq_flags_p = &__rseq_flags; /* Offset from the thread pointer to the rseq area. */ ptrdiff_t rseq_offset; @@ -155,9 +163,17 @@ unsigned int get_rseq_feature_size(void) static __attribute__((constructor)) void rseq_init(void) { - libc_rseq_offset_p = dlsym(RTLD_NEXT, "__rseq_offset"); - libc_rseq_size_p = dlsym(RTLD_NEXT, "__rseq_size"); - libc_rseq_flags_p = dlsym(RTLD_NEXT, "__rseq_flags"); + /* + * If the libc's registered rseq size isn't already valid, it may be + * because the binary is dynamically linked and not necessarily due to + * libc not having registered a restartable sequence. Try to find the + * symbols if that's the case. + */ + if (!*libc_rseq_size_p) { + libc_rseq_offset_p = dlsym(RTLD_NEXT, "__rseq_offset"); + libc_rseq_size_p = dlsym(RTLD_NEXT, "__rseq_size"); + libc_rseq_flags_p = dlsym(RTLD_NEXT, "__rseq_flags"); + } if (libc_rseq_size_p && libc_rseq_offset_p && libc_rseq_flags_p && *libc_rseq_size_p != 0) { /* rseq registration owned by glibc */ -- cgit v1.2.3 From 625646aede90554fed8d46fd0e081238e071ac5e Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 11 Jul 2023 16:01:26 -0700 Subject: KVM: selftests: Use pread() to read binary stats header Use pread() with an explicit offset when reading the header and the header name for a binary stats fd so that the common helper and the binary stats test don't subtly rely on the file effectively being untouched, e.g. to allow multiple reads of the header, name, etc. Signed-off-by: Sean Christopherson Message-Id: <20230711230131.648752-3-seanjc@google.com> Signed-off-by: Paolo Bonzini --- tools/testing/selftests/kvm/include/kvm_util_base.h | 6 ++++-- tools/testing/selftests/kvm/kvm_binary_stats_test.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 07732a157ccd..eb1ff597bcca 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -362,8 +362,10 @@ static inline void read_stats_header(int stats_fd, struct kvm_stats_header *head { ssize_t ret; - ret = read(stats_fd, header, sizeof(*header)); - TEST_ASSERT(ret == sizeof(*header), "Read stats header"); + ret = pread(stats_fd, header, sizeof(*header), 0); + TEST_ASSERT(ret == sizeof(*header), + "Failed to read '%lu' header bytes, ret = '%ld'", + sizeof(*header), ret); } struct kvm_stats_desc *read_stats_descriptors(int stats_fd, diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index a7001e29dc06..eae99d0e8377 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -43,8 +43,10 @@ static void stats_test(int stats_fd) id = malloc(header.name_size); TEST_ASSERT(id, "Allocate memory for id string"); - ret = read(stats_fd, id, header.name_size); - TEST_ASSERT(ret == header.name_size, "Read id string"); + ret = pread(stats_fd, id, header.name_size, sizeof(header)); + TEST_ASSERT(ret == header.name_size, + "Expected header size '%u', read '%lu' bytes", + header.name_size, ret); /* Check id string, that should start with "kvm" */ TEST_ASSERT(!strncmp(id, "kvm", 3) && strlen(id) < header.name_size, -- cgit v1.2.3 From 87d53582bc8be30a11654a45fac0a257ed830ea8 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 11 Jul 2023 16:01:27 -0700 Subject: KVM: selftests: Clean up stats fd in common stats_test() helper Move the stats fd cleanup code into stats_test() and drop the superfluous vm_stats_test() and vcpu_stats_test() helpers in order to decouple creation of the stats file from consuming/testing the file (deduping code is a bonus). This will make it easier to test various edge cases related to stats, e.g. that userspace can dup() a stats fd, that userspace can have multiple stats files for a singleVM/vCPU, etc. Signed-off-by: Sean Christopherson Message-Id: <20230711230131.648752-4-seanjc@google.com> Signed-off-by: Paolo Bonzini --- .../testing/selftests/kvm/kvm_binary_stats_test.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index eae99d0e8377..f02663711c90 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -167,23 +167,7 @@ static void stats_test(int stats_fd) free(stats_data); free(stats_desc); free(id); -} - - -static void vm_stats_test(struct kvm_vm *vm) -{ - int stats_fd = vm_get_stats_fd(vm); - - stats_test(stats_fd); - close(stats_fd); - TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed"); -} - -static void vcpu_stats_test(struct kvm_vcpu *vcpu) -{ - int stats_fd = vcpu_get_stats_fd(vcpu); - stats_test(stats_fd); close(stats_fd); TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed"); } @@ -241,9 +225,11 @@ int main(int argc, char *argv[]) /* Check stats read for every VM and VCPU */ for (i = 0; i < max_vm; ++i) { - vm_stats_test(vms[i]); + stats_test(vm_get_stats_fd(vms[i])); + for (j = 0; j < max_vcpu; ++j) - vcpu_stats_test(vcpus[i * max_vcpu + j]); + stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j])); + ksft_test_result_pass("vm%i\n", i); } -- cgit v1.2.3 From 33b02704071b0972a62ec32516847f91cab6cb8f Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 11 Jul 2023 16:01:28 -0700 Subject: KVM: selftests: Explicitly free vcpus array in binary stats test Explicitly free the all-encompassing vcpus array in the binary stats test so that the test is consistent with respect to freeing all dynamically allocated resources (versus letting them be freed on exit). Signed-off-by: Sean Christopherson Message-Id: <20230711230131.648752-5-seanjc@google.com> Signed-off-by: Paolo Bonzini --- tools/testing/selftests/kvm/kvm_binary_stats_test.c | 1 + 1 file changed, 1 insertion(+) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index f02663711c90..874fa5092551 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -236,6 +236,7 @@ int main(int argc, char *argv[]) for (i = 0; i < max_vm; ++i) kvm_vm_free(vms[i]); free(vms); + free(vcpus); ksft_finished(); /* Print results and exit() accordingly */ } -- cgit v1.2.3 From 47d1be8a78fb587fe5a8f0873244476700171403 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 11 Jul 2023 16:01:29 -0700 Subject: KVM: selftests: Verify userspace can create "redundant" binary stats files Verify that KVM doesn't artificially limit KVM_GET_STATS_FD to a single file per VM/vCPU. There's no known use case for getting multiple stats fds, but it should work, and more importantly creating multiple files will make it easier to test that KVM correct manages VM refcounts for stats files. Signed-off-by: Sean Christopherson Message-Id: <20230711230131.648752-6-seanjc@google.com> Signed-off-by: Paolo Bonzini --- .../testing/selftests/kvm/kvm_binary_stats_test.c | 25 ++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index 874fa5092551..653f10d8fb7c 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -185,6 +185,7 @@ static void stats_test(int stats_fd) int main(int argc, char *argv[]) { + int vm_stats_fds, *vcpu_stats_fds; int i, j; struct kvm_vcpu **vcpus; struct kvm_vm **vms; @@ -217,18 +218,37 @@ int main(int argc, char *argv[]) vcpus = malloc(sizeof(struct kvm_vcpu *) * max_vm * max_vcpu); TEST_ASSERT(vcpus, "Allocate memory for storing vCPU pointers"); + /* + * Not per-VM as the array is populated, used, and invalidated within a + * single for-loop iteration. + */ + vcpu_stats_fds = calloc(max_vm, sizeof(*vcpu_stats_fds)); + TEST_ASSERT(vcpu_stats_fds, "Allocate memory for VM stats fds"); + for (i = 0; i < max_vm; ++i) { vms[i] = vm_create_barebones(); for (j = 0; j < max_vcpu; ++j) vcpus[i * max_vcpu + j] = __vm_vcpu_add(vms[i], j); } - /* Check stats read for every VM and VCPU */ + /* + * Check stats read for every VM and vCPU, with a variety of testcases. + * Note, stats_test() closes the passed in stats fd. + */ for (i = 0; i < max_vm; ++i) { + vm_stats_fds = vm_get_stats_fd(vms[i]); + + /* Verify userspace can instantiate multiple stats files. */ stats_test(vm_get_stats_fd(vms[i])); - for (j = 0; j < max_vcpu; ++j) + for (j = 0; j < max_vcpu; ++j) { + vcpu_stats_fds[j] = vcpu_get_stats_fd(vcpus[i * max_vcpu + j]); stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j])); + } + + stats_test(vm_stats_fds); + for (j = 0; j < max_vcpu; ++j) + stats_test(vcpu_stats_fds[j]); ksft_test_result_pass("vm%i\n", i); } @@ -237,6 +257,7 @@ int main(int argc, char *argv[]) kvm_vm_free(vms[i]); free(vms); free(vcpus); + free(vcpu_stats_fds); ksft_finished(); /* Print results and exit() accordingly */ } -- cgit v1.2.3 From 65f1f57f35e5e833c879a7afb9c862c603695917 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 11 Jul 2023 16:01:30 -0700 Subject: KVM: selftests: Verify stats fd can be dup()'d and read Expand the binary stats test to verify that a stats fd can be dup()'d and read, to (very) roughly simulate userspace passing around the file. Adding the dup() test is primarily an intermediate step towards verifying that userspace can read VM/vCPU stats before _and_ after userspace closes its copy of the VM fd; the dup() test itself is only mildly interesting. Signed-off-by: Sean Christopherson Message-Id: <20230711230131.648752-7-seanjc@google.com> Signed-off-by: Paolo Bonzini --- tools/testing/selftests/kvm/kvm_binary_stats_test.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index 653f10d8fb7c..5317e27b77d0 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -232,17 +232,23 @@ int main(int argc, char *argv[]) } /* - * Check stats read for every VM and vCPU, with a variety of testcases. + * Check stats read for every VM and vCPU, with a variety of flavors. * Note, stats_test() closes the passed in stats fd. */ for (i = 0; i < max_vm; ++i) { + /* + * Verify that creating multiple userspace references to a + * single stats file works and doesn't cause explosions. + */ vm_stats_fds = vm_get_stats_fd(vms[i]); + stats_test(dup(vm_stats_fds)); /* Verify userspace can instantiate multiple stats files. */ stats_test(vm_get_stats_fd(vms[i])); for (j = 0; j < max_vcpu; ++j) { vcpu_stats_fds[j] = vcpu_get_stats_fd(vcpus[i * max_vcpu + j]); + stats_test(dup(vcpu_stats_fds[j])); stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j])); } -- cgit v1.2.3 From 211c0189ea18648a0cf23dea9f4ed745bc9252f6 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 11 Jul 2023 16:01:31 -0700 Subject: KVM: selftests: Verify stats fd is usable after VM fd has been closed Verify that VM and vCPU binary stats files are usable even after userspace has put its last direct reference to the VM. This is a regression test for a UAF bug where KVM didn't gift the stats files a reference to the VM. Signed-off-by: Sean Christopherson Message-Id: <20230711230131.648752-8-seanjc@google.com> Signed-off-by: Paolo Bonzini --- tools/testing/selftests/kvm/kvm_binary_stats_test.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index 5317e27b77d0..698c1cfa3111 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -252,6 +252,14 @@ int main(int argc, char *argv[]) stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j])); } + /* + * Close the VM fd and redo the stats tests. KVM should gift a + * reference (to the VM) to each stats fd, i.e. stats should + * still be accessible even after userspace has put its last + * _direct_ reference to the VM. + */ + kvm_vm_free(vms[i]); + stats_test(vm_stats_fds); for (j = 0; j < max_vcpu; ++j) stats_test(vcpu_stats_fds[j]); @@ -259,8 +267,6 @@ int main(int argc, char *argv[]) ksft_test_result_pass("vm%i\n", i); } - for (i = 0; i < max_vm; ++i) - kvm_vm_free(vms[i]); free(vms); free(vcpus); free(vcpu_stats_fds); -- cgit v1.2.3 From 5a7591176c47cce363c1eed704241e5d1c42c5a6 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Tue, 13 Jun 2023 13:30:37 -0700 Subject: KVM: selftests: Expand x86's sregs test to cover illegal CR0 values Add coverage to x86's set_sregs_test to verify KVM rejects vendor-agnostic illegal CR0 values, i.e. CR0 values whose legality doesn't depend on the current VMX mode. KVM historically has neglected to reject bad CR0s from userspace, i.e. would happily accept a completely bogus CR0 via KVM_SET_SREGS{2}. Punt VMX specific subtests to future work, as they would require quite a bit more effort, and KVM gets coverage for CR0 checks in general through other means, e.g. KVM-Unit-Tests. Signed-off-by: Sean Christopherson Message-Id: <20230613203037.1968489-4-seanjc@google.com> Signed-off-by: Paolo Bonzini --- .../testing/selftests/kvm/x86_64/set_sregs_test.c | 70 ++++++++++++---------- 1 file changed, 39 insertions(+), 31 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/kvm/x86_64/set_sregs_test.c b/tools/testing/selftests/kvm/x86_64/set_sregs_test.c index a284fcef6ed7..3610981d9162 100644 --- a/tools/testing/selftests/kvm/x86_64/set_sregs_test.c +++ b/tools/testing/selftests/kvm/x86_64/set_sregs_test.c @@ -22,26 +22,25 @@ #include "kvm_util.h" #include "processor.h" -static void test_cr4_feature_bit(struct kvm_vcpu *vcpu, struct kvm_sregs *orig, - uint64_t feature_bit) -{ - struct kvm_sregs sregs; - int rc; - - /* Skip the sub-test, the feature is supported. */ - if (orig->cr4 & feature_bit) - return; - - memcpy(&sregs, orig, sizeof(sregs)); - sregs.cr4 |= feature_bit; - - rc = _vcpu_sregs_set(vcpu, &sregs); - TEST_ASSERT(rc, "KVM allowed unsupported CR4 bit (0x%lx)", feature_bit); - - /* Sanity check that KVM didn't change anything. */ - vcpu_sregs_get(vcpu, &sregs); - TEST_ASSERT(!memcmp(&sregs, orig, sizeof(sregs)), "KVM modified sregs"); -} +#define TEST_INVALID_CR_BIT(vcpu, cr, orig, bit) \ +do { \ + struct kvm_sregs new; \ + int rc; \ + \ + /* Skip the sub-test, the feature/bit is supported. */ \ + if (orig.cr & bit) \ + break; \ + \ + memcpy(&new, &orig, sizeof(sregs)); \ + new.cr |= bit; \ + \ + rc = _vcpu_sregs_set(vcpu, &new); \ + TEST_ASSERT(rc, "KVM allowed invalid " #cr " bit (0x%lx)", bit); \ + \ + /* Sanity check that KVM didn't change anything. */ \ + vcpu_sregs_get(vcpu, &new); \ + TEST_ASSERT(!memcmp(&new, &orig, sizeof(new)), "KVM modified sregs"); \ +} while (0) static uint64_t calc_supported_cr4_feature_bits(void) { @@ -80,7 +79,7 @@ int main(int argc, char *argv[]) struct kvm_vcpu *vcpu; struct kvm_vm *vm; uint64_t cr4; - int rc; + int rc, i; /* * Create a dummy VM, specifically to avoid doing KVM_SET_CPUID2, and @@ -92,6 +91,7 @@ int main(int argc, char *argv[]) vcpu_sregs_get(vcpu, &sregs); + sregs.cr0 = 0; sregs.cr4 |= calc_supported_cr4_feature_bits(); cr4 = sregs.cr4; @@ -103,16 +103,24 @@ int main(int argc, char *argv[]) sregs.cr4, cr4); /* Verify all unsupported features are rejected by KVM. */ - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_UMIP); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_LA57); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_VMXE); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_SMXE); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_FSGSBASE); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_PCIDE); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_OSXSAVE); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_SMEP); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_SMAP); - test_cr4_feature_bit(vcpu, &sregs, X86_CR4_PKE); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_UMIP); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_LA57); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_VMXE); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_SMXE); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_FSGSBASE); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_PCIDE); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_OSXSAVE); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_SMEP); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_SMAP); + TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_PKE); + + for (i = 32; i < 64; i++) + TEST_INVALID_CR_BIT(vcpu, cr0, sregs, BIT(i)); + + /* NW without CD is illegal, as is PG without PE. */ + TEST_INVALID_CR_BIT(vcpu, cr0, sregs, X86_CR0_NW); + TEST_INVALID_CR_BIT(vcpu, cr0, sregs, X86_CR0_PG); + kvm_vm_free(vm); /* Create a "real" VM and verify APIC_BASE can be set. */ -- cgit v1.2.3