| 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
| ||
|
Message-ID: <d28efcbd-911a-4ce3-9cb8-3d5b5a872c4d@amd.com>
Date: Thu, 11 Jul 2024 11:27:05 -0500
From: "Sampat, Pratik Rajesh" <pratikrajesh.sampat@....com>
To: Peter Gonda <pgonda@...gle.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 7/11/2024 10:57 AM, Peter Gonda wrote:
> 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?
>
Ack. I can add that comment.
>> +
>> + 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?
>
Ack.
Yes, it could. Maybe the check I was speaking about earlier about SMT
can be made here as well and based on that we decide if this should fail
or pass.
>> +
>> + 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?
>
We could. Looking at loop however, we also go through 0x2 which is
undefined so I thought we were already taking care of the negative test
case here. Having said that, I have no issues in adding one more case
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..."
>
Ack. This double negative is used in a couple of more places. Will clean
them up too.
>> + 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.
>
Yes you're absolutely right. It's technically a correct page type just
not set up correctly to be used this way so we should see it fail.
>> + 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."
>
Thanks for catching this. Will fix the comment.
>> + 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?
>
Ack.
Thanks again for the review
Powered by blists - more mailing lists