| Message ID | 20220719234950.3612318-4-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MSR filtering / exiting flag cleanup | expand |
On Tue, Jul 19, 2022, Aaron Lewis wrote: > When using the flags in KVM_X86_SET_MSR_FILTER and > KVM_CAP_X86_USER_SPACE_MSR it is expected that an attempt to write to > any of the unused bits will fail. Add testing to walk over every bit > in each of the flag fields in MSR filtering / exiting to verify that > happens. > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > .../kvm/x86_64/userspace_msr_exit_test.c | 95 +++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > index f84dc37426f5..3b4ad16cc982 100644 > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void) > kvm_vm_free(vm); > } > > +static void test_results(int rc, const char *scmd, bool expected_success) Rather than pass in "success expected", pass in the actual value and the valid mask. Then you can spit out the problematic value in the assert and be kind to future debuggers. And similarly, make the __vm_ioctl() call here instead of in the "caller" and name this __test_ioctl() (rename as necessary, see below) to show it's relationship with the macro. > +{ > + int expected_rc; > + > + expected_rc = expected_success ? 0 : -1; > + TEST_ASSERT(rc == expected_rc, > + "Unexpected result from '%s', rc: %d, expected rc: %d.", > + scmd, rc, expected_rc); > + TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL), > + "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n" > + " got rc: %d, errno: %d", > + EINVAL, rc, errno); > +} > + > +#define test_ioctl(vm, cmd, arg, expected_success) \ As above, just do e.g. #define test_ioctl(vm, cmd, arg, val, valid_mask) \ __test_ioctl(vm, cmd, arg, #cmd, val, valid_mask) Though it might be worth using a more verbose name? E.g. test_msr_filtering_ioctl()? Hmm, but I guess KVM_CAP_X86_USER_SPACE_MSR isn't technically filtering. test_user_exit_msr_ioctl()? Not a big deal if that's too wordy. > +({ \ > + int rc = __vm_ioctl(vm, cmd, arg); \ > + \ > + test_results(rc, #cmd, expected_success); \ > +}) > +#define FLAG (1ul << i) No. :-) First, silently consuming local variables is Evil with a capital E. Second, just use BIT() or BIT_ULL(). > +/* Test that attempts to write to the unused bits in a flag fails. */ > +static void test_flags(void) For this one, definitely use a more verbose name, even if it seems stupidly redundant. test_user_msr_exit_flags()? > +{ > + struct kvm_vcpu *vcpu; > + struct kvm_vm *vm; > + > + vm = vm_create_with_one_vcpu(&vcpu, NULL); > + > + /* Test flags for KVM_CAP_X86_USER_SPACE_MSR. */ > + run_user_space_msr_flag_test(vm); > + > + /* Test flags and range flags for KVM_X86_SET_MSR_FILTER. */ > + run_msr_filter_flag_test(vm); > + > + kvm_vm_free(vm); > +} > + > int main(int argc, char *argv[]) > { > /* Tell stdout not to buffer its content */ > @@ -745,5 +838,7 @@ int main(int argc, char *argv[]) > > test_msr_permission_bitmap(); > > + test_flags(); > + > return 0; > } > -- > 2.37.1.359.gd136c6c3e2-goog >
> > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void) > > kvm_vm_free(vm); > > } > > > > +static void test_results(int rc, const char *scmd, bool expected_success) > > Rather than pass in "success expected", pass in the actual value and the valid > mask. Then you can spit out the problematic value in the assert and be kind to > future debuggers. > > And similarly, make the __vm_ioctl() call here instead of in the "caller" and name > this __test_ioctl() (rename as necessary, see below) to show it's relationship with > the macro. The other comments look good. I'll update. This one is a bit tricky though. I did originally have __vm_ioctl() in test_results() (or whatever name it will end up with), but the static assert in kvm_do_ioctl() gave me problems. Unless I make test_results() a macro, I have to force cmd to a uint64_t or something other than a literal, then I get this: include/kvm_util_base.h:190:39: error: expression in static assertion is not constant 190 | static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) == _IOC_SIZE(cmd), ""); \ | ^ include/kvm_util_base.h:213:9: note: in expansion of macro ‘kvm_do_ioctl’ 213 | kvm_do_ioctl((vm)->fd, cmd, arg); \ That's not the only problem. In order to pass 'arg' in I would have to pass it as a void *, making sizeof(*arg) wrong. Being that the ioctl call was the first thing I did in that function I opted to make it a part of test_ioctl() rather than making test_results() a macro. If only C had templates :) > > > +{ > > + int expected_rc; > > + > > + expected_rc = expected_success ? 0 : -1; > > + TEST_ASSERT(rc == expected_rc, > > + "Unexpected result from '%s', rc: %d, expected rc: %d.", > > + scmd, rc, expected_rc); > > + TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL), > > + "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n" > > + " got rc: %d, errno: %d", > > + EINVAL, rc, errno); > > +} > > + > > +#define test_ioctl(vm, cmd, arg, expected_success) \ >
On Thu, Jul 21, 2022, Aaron Lewis wrote: > > > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > > > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c > > > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void) > > > kvm_vm_free(vm); > > > } > > > > > > +static void test_results(int rc, const char *scmd, bool expected_success) > > > > Rather than pass in "success expected", pass in the actual value and the valid > > mask. Then you can spit out the problematic value in the assert and be kind to > > future debuggers. > > > > And similarly, make the __vm_ioctl() call here instead of in the "caller" and name > > this __test_ioctl() (rename as necessary, see below) to show it's relationship with > > the macro. > > The other comments look good. I'll update. > > This one is a bit tricky though. I did originally have __vm_ioctl() > in test_results() (or whatever name it will end up with), but the > static assert in kvm_do_ioctl() gave me problems. Unless I make > test_results() a macro, I have to force cmd to a uint64_t or something > other than a literal, then I get this: > > include/kvm_util_base.h:190:39: error: expression in static assertion > is not constant > 190 | static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) == > _IOC_SIZE(cmd), ""); \ > | ^ > include/kvm_util_base.h:213:9: note: in expansion of macro ‘kvm_do_ioctl’ > 213 | kvm_do_ioctl((vm)->fd, cmd, arg); \ > > That's not the only problem. In order to pass 'arg' in I would have > to pass it as a void *, making sizeof(*arg) wrong. > > Being that the ioctl call was the first thing I did in that function I > opted to make it a part of test_ioctl() rather than making > test_results() a macro. > > If only C had templates :) Eh, what C++ can do with templates, C can usually do with macros :-) Sans backslashes, I think this can simply be: #define test_user_exit_msr_ioctl(vm, cmd, arg, val, valid_mask) ({ int r = __vm_ioctl(vm, cmd, arg); if (val & valid_mask) TEST_ASSERT(!r, KVM_IOCTL_ERROR(cmd, r)); else TEST_ASSERT(r == -1 && errno == EINVAL, "Wanted EINVAL with val = 0x%llx, got rc: %i errno: %i (%s)", val, r, errno, strerror(errno)) }) It doesn't print "val" when success is expected, but I'm ok with that. Though I suspect that adding a common macro to print additional info on an unexpected ioctl() result would be useful for other tests.
diff --git a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c index f84dc37426f5..3b4ad16cc982 100644 --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void) kvm_vm_free(vm); } +static void test_results(int rc, const char *scmd, bool expected_success) +{ + int expected_rc; + + expected_rc = expected_success ? 0 : -1; + TEST_ASSERT(rc == expected_rc, + "Unexpected result from '%s', rc: %d, expected rc: %d.", + scmd, rc, expected_rc); + TEST_ASSERT(!rc || (rc == -1 && errno == EINVAL), + "Failures are expected to have rc == -1 && errno == EINVAL(%d),\n" + " got rc: %d, errno: %d", + EINVAL, rc, errno); +} + +#define test_ioctl(vm, cmd, arg, expected_success) \ +({ \ + int rc = __vm_ioctl(vm, cmd, arg); \ + \ + test_results(rc, #cmd, expected_success); \ +}) +#define FLAG (1ul << i) + +static void run_user_space_msr_flag_test(struct kvm_vm *vm) +{ + struct kvm_enable_cap cap = { .cap = KVM_CAP_X86_USER_SPACE_MSR }; + int nflags = sizeof(cap.args[0]) * BITS_PER_BYTE; + int rc; + int i; + + rc = kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR); + TEST_ASSERT(rc, "KVM_CAP_X86_USER_SPACE_MSR is available"); + + for (i = 0; i < nflags; i++) { + cap.args[0] = FLAG; + test_ioctl(vm, KVM_ENABLE_CAP, &cap, + FLAG & KVM_MSR_EXIT_REASON_VALID_MASK); + } +} + +static void run_msr_filter_flag_test(struct kvm_vm *vm) +{ + u64 deny_bits = 0; + struct kvm_msr_filter filter = { + .flags = KVM_MSR_FILTER_DEFAULT_ALLOW, + .ranges = { + { + .flags = KVM_MSR_FILTER_READ, + .nmsrs = 1, + .base = 0, + .bitmap = (uint8_t *)&deny_bits, + }, + }, + }; + int nflags; + int rc; + int i; + + rc = kvm_check_cap(KVM_CAP_X86_MSR_FILTER); + TEST_ASSERT(rc, "KVM_CAP_X86_MSR_FILTER is available"); + + nflags = sizeof(filter.flags) * BITS_PER_BYTE; + for (i = 0; i < nflags; i++) { + filter.flags = FLAG; + test_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter, + FLAG & KVM_MSR_FILTER_VALID_MASK); + } + + filter.flags = KVM_MSR_FILTER_DEFAULT_ALLOW; + nflags = sizeof(filter.ranges[0].flags) * BITS_PER_BYTE; + for (i = 0; i < nflags; i++) { + filter.ranges[0].flags = FLAG; + test_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter, + FLAG & KVM_MSR_FILTER_RANGE_VALID_MASK); + } +} + +/* Test that attempts to write to the unused bits in a flag fails. */ +static void test_flags(void) +{ + struct kvm_vcpu *vcpu; + struct kvm_vm *vm; + + vm = vm_create_with_one_vcpu(&vcpu, NULL); + + /* Test flags for KVM_CAP_X86_USER_SPACE_MSR. */ + run_user_space_msr_flag_test(vm); + + /* Test flags and range flags for KVM_X86_SET_MSR_FILTER. */ + run_msr_filter_flag_test(vm); + + kvm_vm_free(vm); +} + int main(int argc, char *argv[]) { /* Tell stdout not to buffer its content */ @@ -745,5 +838,7 @@ int main(int argc, char *argv[]) test_msr_permission_bitmap(); + test_flags(); + return 0; }
When using the flags in KVM_X86_SET_MSR_FILTER and KVM_CAP_X86_USER_SPACE_MSR it is expected that an attempt to write to any of the unused bits will fail. Add testing to walk over every bit in each of the flag fields in MSR filtering / exiting to verify that happens. Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- .../kvm/x86_64/userspace_msr_exit_test.c | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+)