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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMkAt6ojzv+FYV5Hnvy9whf-TRTxht2C2y4Xnx_VsM_O5G50eA@mail.gmail.com>
Date: Thu, 11 Jul 2024 09:57:07 -0600
From: Peter Gonda <pgonda@...gle.com>
To: "Pratik R. Sampat" <pratikrajesh.sampat@....com>
Cc: kvm@...r.kernel.org, shuah@...nel.org, thomas.lendacky@....com, 
	michael.roth@....com, seanjc@...gle.com, pbonzini@...hat.com, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 4/5] selftests: KVM: SNP IOCTL test

On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
<pratikrajesh.sampat@....com> wrote:
>
> Introduce testing of SNP ioctl calls. This patch includes both positive
> and negative tests of various parameters such as flags, page types and
> policies.
>
> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@....com>

Tested-by: Peter Gonda <pgonda@...gle.com>

> ---
>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
>  1 file changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> index 500c67b3793b..1d5c275c11b3 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
> @@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>         kvm_vm_free(vm);
>  }
>
> +static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
> +{
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       int ret;
> +
> +       vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +       ret = snp_vm_launch(vm, policy, flags);
> +       kvm_vm_free(vm);
> +
> +       return ret;
> +}
> +
> +static void test_snp_launch_start(uint32_t type, uint64_t policy)
> +{
> +       uint8_t i;
> +       int ret;
> +
> +       ret = spawn_snp_launch_start(type, policy, 0);
> +       TEST_ASSERT(!ret,
> +                   "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");
> +
> +       for (i = 1; i < 8; i++) {
> +               ret = spawn_snp_launch_start(type, policy, BIT(i));
> +               TEST_ASSERT(ret && errno == EINVAL,
> +                           "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");
> +       }

To save readers sometime do we want to comment that flags must be zero?

> +
> +       ret = spawn_snp_launch_start(type, 0, 0);
> +       TEST_ASSERT(ret && errno == EINVAL,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
> +
> +       ret = spawn_snp_launch_start(type, SNP_POLICY_SMT, 0);
> +       TEST_ASSERT(ret && errno == EINVAL,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
> +
> +       ret = spawn_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0);
> +       TEST_ASSERT(ret && errno == EINVAL,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");

Ditto on SMT comment, this could pass if SMT was disabled right?

> +
> +       ret = spawn_snp_launch_start(type, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
> +                                    (255 * SNP_POLICY_ABI_MAJOR) |
> +                                    (255 * SNP_POLICY_ABI_MINOR), 0);
> +       TEST_ASSERT(ret && errno == EIO,
> +                   "KVM_SEV_SNP_LAUNCH_START should fail, invalid version.");
> +}
> +
> +static void test_snp_launch_update(uint32_t type, uint64_t policy)
> +{
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       int ret;
> +
> +       for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID; pgtype++) {

Do we want to test KVM_SEV_SNP_PAGE_TYPE_CPUID+1 to make sure that fails?

> +               vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +               snp_vm_launch(vm, policy, 0);
> +               ret = snp_vm_launch_update(vm, pgtype);
> +
> +               switch (pgtype) {
> +               case KVM_SEV_SNP_PAGE_TYPE_NORMAL:
> +               case KVM_SEV_SNP_PAGE_TYPE_ZERO:
> +               case KVM_SEV_SNP_PAGE_TYPE_UNMEASURED:
> +               case KVM_SEV_SNP_PAGE_TYPE_SECRETS:
> +                       TEST_ASSERT(!ret,
> +                                   "KVM_SEV_SNP_LAUNCH_UPDATE should not fail, invalid Page type.");

Double negative maybe: "KVM_SEV_SNP_LAUNCH_UPDATE should succeed..."

> +                       break;
> +               case KVM_SEV_SNP_PAGE_TYPE_CPUID:
> +                       TEST_ASSERT(ret && errno == EIO,
> +                                   "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");

This is a valid page type right? But I think the error is from the ASP
due to the page being malformed for a CPUID page.

> +                       break;
> +               default:
> +                       TEST_ASSERT(ret && errno == EINVAL,
> +                                   "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");
> +               }
> +
> +               kvm_vm_free(vm);
> +       }
> +}
> +
> +void test_snp_launch_finish(uint32_t type, uint64_t policy)
> +{
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_vm *vm;
> +       int ret;
> +
> +       vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +       snp_vm_launch(vm, policy, 0);
> +       snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
> +       ret = snp_vm_launch_finish(vm, 0);
> +       TEST_ASSERT(!ret,
> +                   "KVM_SEV_SNP_LAUNCH_FINISH should not fail, invalid flag.");

Comment is wrong, maybe: "KVM_SEV_SNP_LAUNCH_FINISH should not fail."

> +       kvm_vm_free(vm);
> +
> +       for (int i = 1; i < 16; i++) {
> +               vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> +               snp_vm_launch(vm, policy, 0);
> +               snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
> +               ret = snp_vm_launch_finish(vm, BIT(i));
> +               TEST_ASSERT(ret && errno == EINVAL,
> +                           "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag.");
> +               kvm_vm_free(vm);

To save readers sometime do we want to comment that flags must be zero?

> +       }
> +}
> +
> +static void test_sev_ioctl(void *guest_code, uint32_t type, uint64_t policy)
> +{
> +       if (type == KVM_X86_SNP_VM) {
> +               test_snp_launch_start(type, policy);
> +               test_snp_launch_update(type, policy);
> +               test_snp_launch_finish(type, policy);
> +
> +               return;
> +       }
> +
> +       test_sev_launch(guest_code, type, policy);
> +}
> +
>  static void test_sev(void *guest_code, uint32_t type, uint64_t policy)
>  {
>         struct kvm_vcpu *vcpu;
>         struct kvm_vm *vm;
>         struct ucall uc;
>
> -       test_sev_launch(guest_code, type, policy);
> +       test_sev_ioctl(guest_code, type, policy);
>
>         vm = vm_sev_create_with_one_vcpu(type, guest_code, &vcpu);
>
> --
> 2.34.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ