Re: [PATCH v3 2/8] x86/sev: Do not require Hypervisor CPUID bit for SEV guests

[Tom Lendacky <thomas.lendacky@amd.com>]

On 3/12/21 6:38 AM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> A malicious hypervisor could disable the CPUID intercept for an SEV or
> SEV-ES guest and trick it into the no-SEV boot path, where it could
> potentially reveal secrets. This is not an issue for SEV-SNP guests,
> as the CPUID intercept can't be disabled for those.
> 
> Remove the Hypervisor CPUID bit check from the SEV detection code to
> protect against this kind of attack and add a Hypervisor bit equals
> zero check to the SME detection path to prevent non-SEV guests from
> trying to enable SME.
> 
> This handles the following cases:
> 
> 	1) SEV(-ES) guest where CPUID intercept is disabled. The guest
> 	   will still see leaf 0x8000001f and the SEV bit. It can
> 	   retrieve the C-bit and boot normally.
> 
> 	2) Non-SEV guests with intercepted CPUID will check SEV_STATUS
> 	   MSR and find it 0 and will try to enable SME. This will
> 	   fail when the guest finds MSR_K8_SYSCFG to be zero, as it
> 	   is emulated by KVM. But we can't rely on that, as there
> 	   might be other hypervisors which return this MSR with bit
> 	   23 set. The Hypervisor bit check will prevent that the
> 	   guest tries to enable SME in this case.
> 
> 	3) Non-SEV guests on SEV capable hosts with CPUID intercept
> 	   disabled (by a malicious hypervisor) will try to boot into
> 	   the SME path. This will fail, but it is also not considered
> 	   a problem because non-encrypted guests have no protection
> 	   against the hypervisor anyway.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/boot/compressed/mem_encrypt.S |  6 -----
>  arch/x86/kernel/sev-es-shared.c        |  6 +----
>  arch/x86/mm/mem_encrypt_identity.c     | 35 ++++++++++++++------------
>  3 files changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> index aa561795efd1..a6dea4e8a082 100644
> --- a/arch/x86/boot/compressed/mem_encrypt.S
> +++ b/arch/x86/boot/compressed/mem_encrypt.S
> @@ -23,12 +23,6 @@ SYM_FUNC_START(get_sev_encryption_bit)
>  	push	%ecx
>  	push	%edx
>  
> -	/* Check if running under a hypervisor */
> -	movl	$1, %eax
> -	cpuid
> -	bt	$31, %ecx		/* Check the hypervisor bit */
> -	jnc	.Lno_sev
> -
>  	movl	$0x80000000, %eax	/* CPUID to check the highest leaf */
>  	cpuid
>  	cmpl	$0x8000001f, %eax	/* See if 0x8000001f is available */
> diff --git a/arch/x86/kernel/sev-es-shared.c b/arch/x86/kernel/sev-es-shared.c
> index cdc04d091242..387b71669818 100644
> --- a/arch/x86/kernel/sev-es-shared.c
> +++ b/arch/x86/kernel/sev-es-shared.c
> @@ -186,7 +186,6 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>  	 * make it accessible to the hypervisor.
>  	 *
>  	 * In particular, check for:
> -	 *	- Hypervisor CPUID bit
>  	 *	- Availability of CPUID leaf 0x8000001f
>  	 *	- SEV CPUID bit.
>  	 *
> @@ -194,10 +193,7 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>  	 * can't be checked here.
>  	 */
>  
> -	if ((fn == 1 && !(regs->cx & BIT(31))))
> -		/* Hypervisor bit */
> -		goto fail;
> -	else if (fn == 0x80000000 && (regs->ax < 0x8000001f))
> +	if (fn == 0x80000000 && (regs->ax < 0x8000001f))
>  		/* SEV leaf check */
>  		goto fail;
>  	else if ((fn == 0x8000001f && !(regs->ax & BIT(1))))
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index 6c5eb6f3f14f..a19374d26101 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -503,14 +503,10 @@ void __init sme_enable(struct boot_params *bp)
>  
>  #define AMD_SME_BIT	BIT(0)
>  #define AMD_SEV_BIT	BIT(1)
> -	/*
> -	 * Set the feature mask (SME or SEV) based on whether we are
> -	 * running under a hypervisor.
> -	 */
> -	eax = 1;
> -	ecx = 0;
> -	native_cpuid(&eax, &ebx, &ecx, &edx);
> -	feature_mask = (ecx & BIT(31)) ? AMD_SEV_BIT : AMD_SME_BIT;
> +
> +	/* Check the SEV MSR whether SEV or SME is enabled */
> +	sev_status   = __rdmsr(MSR_AMD64_SEV);
> +	feature_mask = (sev_status & MSR_AMD64_SEV_ENABLED) ? AMD_SEV_BIT : AMD_SME_BIT;
>  
>  	/*
>  	 * Check for the SME/SEV feature:
> @@ -530,19 +526,26 @@ void __init sme_enable(struct boot_params *bp)
>  
>  	/* Check if memory encryption is enabled */
>  	if (feature_mask == AMD_SME_BIT) {
> +		/*
> +		 * No SME if Hypervisor bit is set. This check is here to
> +		 * prevent a guest from trying to enable SME. For running as a
> +		 * KVM guest the MSR_K8_SYSCFG will be sufficient, but there
> +		 * might be other hypervisors which emulate that MSR as non-zero
> +		 * or even pass it through to the guest.
> +		 * A malicious hypervisor can still trick a guest into this
> +		 * path, but there is no way to protect against that.
> +		 */
> +		eax = 1;
> +		ecx = 0;
> +		native_cpuid(&eax, &ebx, &ecx, &edx);
> +		if (ecx & BIT(31))
> +			return;
> +
>  		/* For SME, check the SYSCFG MSR */
>  		msr = __rdmsr(MSR_K8_SYSCFG);
>  		if (!(msr & MSR_K8_SYSCFG_MEM_ENCRYPT))
>  			return;
>  	} else {
> -		/* For SEV, check the SEV MSR */
> -		msr = __rdmsr(MSR_AMD64_SEV);
> -		if (!(msr & MSR_AMD64_SEV_ENABLED))
> -			return;
> -
> -		/* Save SEV_STATUS to avoid reading MSR again */
> -		sev_status = msr;
> -
>  		/* SEV state cannot be controlled by a command line option */
>  		sme_me_mask = me_mask;
>  		sev_enabled = true;
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization