linux-kernel - Re: [PATCH v2 11/11] selftests: kvm: add tests for KVM_SEV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABgObfZGWHM414oq3o6YW=KBNytLGdtDThCdmCWLjPUOAutnjA@mail.gmail.com>
Date: Fri, 23 Feb 2024 19:04:45 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org, michael.roth@....com, 
	aik@....com
Subject: Re: [PATCH v2 11/11] selftests: kvm: add tests for KVM_SEV_INIT2

On Fri, Feb 23, 2024 at 6:18 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Fri, Feb 23, 2024, Paolo Bonzini wrote:
> > diff --git a/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c
> > new file mode 100644
> > index 000000000000..644fd5757041
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_init2_tests.c
> > @@ -0,0 +1,146 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/kvm.h>
> > +#include <linux/psp-sev.h>
> > +#include <stdio.h>
> > +#include <sys/ioctl.h>
> > +#include <stdlib.h>
> > +#include <errno.h>
> > +#include <pthread.h>
> > +
> > +#include "test_util.h"
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "svm_util.h"
> > +#include "kselftest.h"
> > +
> > +#define SVM_SEV_FEAT_DEBUG_SWAP 32u
> > +
> > +/*
> > + * Some features may have hidden dependencies, or may only work
> > + * for certain VM types.  Err on the side of safety and don't
> > + * expect that all supported features can be passed one by one
> > + * to KVM_SEV_INIT2.
> > + *
> > + * (Well, right now there's only one...)
> > + */
> > +#define KNOWN_FEATURES SVM_SEV_FEAT_DEBUG_SWAP
> > +
> > +int kvm_fd;
> > +u64 supported_vmsa_features;
> > +bool have_sev_es;
> > +
> > +static int __sev_ioctl(int vm_fd, int cmd_id, void *data)
> > +{
> > +     struct kvm_sev_cmd cmd = {
> > +             .id = cmd_id,
> > +             .data = (uint64_t)data,
> > +             .sev_fd = open_sev_dev_path_or_exit(),
> > +     };
> > +     int ret;
> > +
> > +     ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
> > +     TEST_ASSERT(ret < 0 || cmd.error == SEV_RET_SUCCESS,
> > +                 "%d failed: fw error: %d\n",
> > +                 cmd_id, cmd.error);
> > +
> > +     return ret;
>
> If you can hold off on v3 until next week, I'll get the SEV+SEV-ES smoke test
> series into a branch and thus kvm-x86/next.  Then this can take advantage of the
> library files and functions that are added there.  I don't know if it will save
> much code, but it'll at least provide a better place to land some of the "library"
> #define and helpers.
>
> https://lore.kernel.org/all/20240223004258.3104051-1-seanjc@google.com

I'll post v3 anyway, but hold on actually committing this until I've
taken a closer look at kvm-x86/next.

> > +     TEST_ASSERT(ret == -1 && errno == EINVAL,
> > +                 "KVM_SEV_INIT2 return code %d, errno: %d (expected EINVAL)",
> > +                 ret, errno);
>
> TEST_ASSERT() will spit out the errno and it's user-friendly name.  I would prefer
> the assert message to explain why failure was expected.  That way readers of the
> code don't need a comment, and runners of failed tests get more info.
>
> Hrm, though that'd require assing in a "const char *msg", which would limit this
> to constant strings and no formatting.  I think that's still a net positive though.

Ok, will do.

>         TEST_ASSERT(ret == -1 && errno == EINVAL,
>                     "KVM_SET_INIT2 should fail, %s.", msg);
>
> > +     kvm_vm_free(vm);
> > +}
> > +
> > +void test_vm_types(void)
> > +{
> > +     test_init2(KVM_X86_SEV_VM, &(struct kvm_sev_init){});
> > +
> > +     if (have_sev_es)
> > +             test_init2(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){});
> > +     else
> > +             test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){});
>
> E.g. this could be something like
>
>                 test_init2_invalid(KVM_X86_SEV_ES_VM, &(struct kvm_sev_init){},
>                                    "SEV-ES unsupported);
>
> Though shouldn't vm_create_barebones_type() fail on the unsupported VM type, not
> KVM_SEV_INIT2?

Yes, this test is broken. vm_create_barebones_type() errors out.

Paolo