linux-kernel - Re: [PATCH v2 04/49] KVM: selftests: Update x86's set_sregs

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: <55ed983e83b419fe0d8063cc41953b9f45fac198.camel@redhat.com>
Date: Thu, 04 Jul 2024 20:55:59 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Hou Wenlong
 <houwenlong.hwl@...group.com>, Kechen Lu <kechenl@...dia.com>, Oliver Upton
 <oliver.upton@...ux.dev>, Binbin Wu <binbin.wu@...ux.intel.com>, Yang
 Weijiang <weijiang.yang@...el.com>, Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 04/49] KVM: selftests: Update x86's set_sregs_test to
 match KVM's CPUID enforcement

On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> Rework x86's set sregs test to verify that KVM enforces CPUID vs. CR4
> features even if userspace hasn't explicitly set guest CPUID.  KVM used to
> allow userspace to set any KVM-supported CR4 value prior to KVM_SET_CPUID2,
> and the test verified that behavior.
> 
> However, the testcase was written purely to verify KVM's existing behavior,
> i.e. was NOT written to match the needs of real world VMMs.
> 
> Opportunistically verify that KVM continues to reject unsupported features
> after KVM_SET_CPUID2 (using KVM_GET_SUPPORTED_CPUID).
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  .../selftests/kvm/x86_64/set_sregs_test.c     | 53 +++++++++++--------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/set_sregs_test.c b/tools/testing/selftests/kvm/x86_64/set_sregs_test.c
> index c021c0795a96..96fd690d479a 100644
> --- a/tools/testing/selftests/kvm/x86_64/set_sregs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/set_sregs_test.c
> @@ -41,13 +41,15 @@ do {										\
>  	TEST_ASSERT(!memcmp(&new, &orig, sizeof(new)), "KVM modified sregs");	\
>  } while (0)
>  
> +#define KVM_ALWAYS_ALLOWED_CR4 (X86_CR4_VME | X86_CR4_PVI | X86_CR4_TSD |	\
> +				X86_CR4_DE | X86_CR4_PSE | X86_CR4_PAE |	\
> +				X86_CR4_MCE | X86_CR4_PGE | X86_CR4_PCE |	\
> +				X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT)
> +
>  static uint64_t calc_supported_cr4_feature_bits(void)
>  {
> -	uint64_t cr4;
> +	uint64_t cr4 = KVM_ALWAYS_ALLOWED_CR4;
>  
> -	cr4 = X86_CR4_VME | X86_CR4_PVI | X86_CR4_TSD | X86_CR4_DE |
> -	      X86_CR4_PSE | X86_CR4_PAE | X86_CR4_MCE | X86_CR4_PGE |
> -	      X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT;
>  	if (kvm_cpu_has(X86_FEATURE_UMIP))
>  		cr4 |= X86_CR4_UMIP;
>  	if (kvm_cpu_has(X86_FEATURE_LA57))
> @@ -72,28 +74,14 @@ static uint64_t calc_supported_cr4_feature_bits(void)
>  	return cr4;
>  }
>  
> -int main(int argc, char *argv[])
> +static void test_cr_bits(struct kvm_vcpu *vcpu, uint64_t cr4)
>  {
>  	struct kvm_sregs sregs;
> -	struct kvm_vcpu *vcpu;
> -	struct kvm_vm *vm;
> -	uint64_t cr4;
>  	int rc, i;
>  
> -	/*
> -	 * Create a dummy VM, specifically to avoid doing KVM_SET_CPUID2, and
> -	 * use it to verify all supported CR4 bits can be set prior to defining
> -	 * the vCPU model, i.e. without doing KVM_SET_CPUID2.
> -	 */
> -	vm = vm_create_barebones();
> -	vcpu = __vm_vcpu_add(vm, 0);
> -
>  	vcpu_sregs_get(vcpu, &sregs);
> -
> -	sregs.cr0 = 0;
> -	sregs.cr4 |= calc_supported_cr4_feature_bits();
> -	cr4 = sregs.cr4;
> -
> +	sregs.cr0 &= ~(X86_CR0_CD | X86_CR0_NW);
> +	sregs.cr4 |= cr4;
>  	rc = _vcpu_sregs_set(vcpu, &sregs);
>  	TEST_ASSERT(!rc, "Failed to set supported CR4 bits (0x%lx)", cr4);
>  
> @@ -101,7 +89,6 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(sregs.cr4 == cr4, "sregs.CR4 (0x%llx) != CR4 (0x%lx)",
>  		    sregs.cr4, cr4);
>  
> -	/* Verify all unsupported features are rejected by KVM. */
>  	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_UMIP);
>  	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_LA57);
>  	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_VMXE);
> @@ -119,10 +106,28 @@ int main(int argc, char *argv[])
>  	/* NW without CD is illegal, as is PG without PE. */
>  	TEST_INVALID_CR_BIT(vcpu, cr0, sregs, X86_CR0_NW);
>  	TEST_INVALID_CR_BIT(vcpu, cr0, sregs, X86_CR0_PG);
> +}
>  
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_sregs sregs;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	int rc;
> +
> +	/*
> +	 * Create a dummy VM, specifically to avoid doing KVM_SET_CPUID2, and
> +	 * use it to verify KVM enforces guest CPUID even if *userspace* never
> +	 * sets CPUID.
> +	 */
> +	vm = vm_create_barebones();
> +	vcpu = __vm_vcpu_add(vm, 0);
> +	test_cr_bits(vcpu, KVM_ALWAYS_ALLOWED_CR4);
>  	kvm_vm_free(vm);
>  
> -	/* Create a "real" VM and verify APIC_BASE can be set. */
> +	/* Create a "real" VM with a fully populated guest CPUID and verify
> +	 * APIC_BASE and all supported CR4 can be set.
> +	 */
>  	vm = vm_create_with_one_vcpu(&vcpu, NULL);
>  
>  	vcpu_sregs_get(vcpu, &sregs);
> @@ -135,6 +140,8 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(!rc, "Couldn't set IA32_APIC_BASE to %llx (valid)",
>  		    sregs.apic_base);
>  
> +	test_cr_bits(vcpu, calc_supported_cr4_feature_bits());
> +
>  	kvm_vm_free(vm);
>  
>  	return 0;


Makes sense.

Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>

Best regards,
	Maxim Levitsky