| Message ID | 20211207201034.1392660-1-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests: sev_migrate_tests: Fix sev_ioctl() | expand |
On Tue, Dec 07, 2021, Peter Gonda wrote: > TEST_ASSERT in SEV ioctl was allowing errors because it checked return > value was good OR the FW error code was OK. This TEST_ASSERT should > require both (aka. AND) values are OK. Removes the LAUNCH_START from the > mirror VM because this call correctly fails because mirror VMs cannot > call this command. This probably should be two separate patches. First remove the bogus LAUNCH_START call, then fix the assert. > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: Marc Orr <marcorr@google.com> > Signed-off-by: Peter Gonda <pgonda@google.com> > --- > tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > index 29b18d565cf4..8e1b1e737cb1 100644 > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > @@ -31,7 +31,7 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data) > int ret; > > ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); > - TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS), > + TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS, > "%d failed: return code: %d, errno: %d, fw error: %d", > cmd_id, ret, errno, cmd.error); Hmm, reading cmd.error could also consume uninitialized data, e.g. if the ioctl() fails before getting into the PSP command, the error message will dump garbage. And theoretically this could get a false negative if the test stack happens to have '0' for cmd.error and KVM neglects to fill cmd.error when the ioctl() succeeds. So in additional to fixing the assert itself, I vote we also do: diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c index 29b18d565cf4..50132e165a8d 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c @@ -26,6 +26,7 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data) struct kvm_sev_cmd cmd = { .id = cmd_id, .data = (uint64_t)data, + .error = -1u, .sev_fd = open_sev_dev_path_or_exit(), }; int ret;
On Tue, Dec 7, 2021 at 2:25 PM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Dec 07, 2021, Peter Gonda wrote: > > TEST_ASSERT in SEV ioctl was allowing errors because it checked return > > value was good OR the FW error code was OK. This TEST_ASSERT should > > require both (aka. AND) values are OK. Removes the LAUNCH_START from the > > mirror VM because this call correctly fails because mirror VMs cannot > > call this command. > > This probably should be two separate patches. First remove the bogus LAUNCH_START > call, then fix the assert. Thanks Sean. I'll split the patch and add your suggestion to the second one. > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Sean Christopherson <seanjc@google.com> > > Cc: Marc Orr <marcorr@google.com> > > Signed-off-by: Peter Gonda <pgonda@google.com> > > --- > > tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > > index 29b18d565cf4..8e1b1e737cb1 100644 > > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > > @@ -31,7 +31,7 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data) > > int ret; > > > > ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); > > - TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS), > > + TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS, > > "%d failed: return code: %d, errno: %d, fw error: %d", > > cmd_id, ret, errno, cmd.error); > > Hmm, reading cmd.error could also consume uninitialized data, e.g. if the ioctl() > fails before getting into the PSP command, the error message will dump garbage. > > And theoretically this could get a false negative if the test stack happens to have > '0' for cmd.error and KVM neglects to fill cmd.error when the ioctl() succeeds. > > So in additional to fixing the assert itself, I vote we also do: > > diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > index 29b18d565cf4..50132e165a8d 100644 > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > @@ -26,6 +26,7 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data) > struct kvm_sev_cmd cmd = { > .id = cmd_id, > .data = (uint64_t)data, > + .error = -1u, > .sev_fd = open_sev_dev_path_or_exit(), > }; > int ret; Good idea will do in the 2/2.
On Tue, Dec 7, 2021 at 12:10 PM Peter Gonda <pgonda@google.com> wrote: > > TEST_ASSERT in SEV ioctl was allowing errors because it checked return > value was good OR the FW error code was OK. This TEST_ASSERT should > require both (aka. AND) values are OK. Removes the LAUNCH_START from the > mirror VM because this call correctly fails because mirror VMs cannot > call this command. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: Marc Orr <marcorr@google.com> > Signed-off-by: Peter Gonda <pgonda@google.com> > --- > tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > index 29b18d565cf4..8e1b1e737cb1 100644 > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c > @@ -31,7 +31,7 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data) > int ret; > > ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); > - TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS), > + TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS, > "%d failed: return code: %d, errno: %d, fw error: %d", > cmd_id, ret, errno, cmd.error); > } > @@ -228,9 +228,6 @@ static void sev_mirror_create(int dst_fd, int src_fd) > static void test_sev_mirror(bool es) > { > struct kvm_vm *src_vm, *dst_vm; > - struct kvm_sev_launch_start start = { > - .policy = es ? SEV_POLICY_ES : 0 > - }; > int i; > > src_vm = sev_vm_create(es); > @@ -241,7 +238,7 @@ static void test_sev_mirror(bool es) > /* Check that we can complete creation of the mirror VM. */ > for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i) > vm_vcpu_add(dst_vm, i); > - sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_START, &start); > + > if (es) > sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL); > > -- > 2.34.1.400.ga245620fadb-goog > +1 to Sean's feedback. Otherwise: Reviewed-by: Marc Orr <marcorr@google.com>
diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c index 29b18d565cf4..8e1b1e737cb1 100644 --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c @@ -31,7 +31,7 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data) int ret; ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd); - TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS), + TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS, "%d failed: return code: %d, errno: %d, fw error: %d", cmd_id, ret, errno, cmd.error); } @@ -228,9 +228,6 @@ static void sev_mirror_create(int dst_fd, int src_fd) static void test_sev_mirror(bool es) { struct kvm_vm *src_vm, *dst_vm; - struct kvm_sev_launch_start start = { - .policy = es ? SEV_POLICY_ES : 0 - }; int i; src_vm = sev_vm_create(es); @@ -241,7 +238,7 @@ static void test_sev_mirror(bool es) /* Check that we can complete creation of the mirror VM. */ for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i) vm_vcpu_add(dst_vm, i); - sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_START, &start); + if (es) sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
TEST_ASSERT in SEV ioctl was allowing errors because it checked return value was good OR the FW error code was OK. This TEST_ASSERT should require both (aka. AND) values are OK. Removes the LAUNCH_START from the mirror VM because this call correctly fails because mirror VMs cannot call this command. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Sean Christopherson <seanjc@google.com> Cc: Marc Orr <marcorr@google.com> Signed-off-by: Peter Gonda <pgonda@google.com> --- tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)