diff mbox series

[v2,5/7] x86/boot/compressed/64: Add CPUID sanity check to 32-bit boot-path

Message ID 20210310084325.12966-6-joro@8bytes.org (mailing list archive)
State New, archived
Headers show
Series x86/seves: Support 32-bit boot path and other updates | expand

Commit Message

Joerg Roedel March 10, 2021, 8:43 a.m. UTC 
From: Joerg Roedel <jroedel@suse.de>

The 32-bit #VC handler has no GHCB and can only handle CPUID exit codes.
It is needed by the early boot code to handle #VC exceptions raised in
verify_cpu() and to get the position of the C bit.

But the CPUID information comes from the hypervisor, which is untrusted
and might return results which trick the guest into the no-SEV boot path
with no C bit set in the page-tables. All data written to memory would
then be unencrypted and could leak sensitive data to the hypervisor.

Add sanity checks to the 32-bit boot #VC handler to make sure the
hypervisor does not pretend that SEV is not enabled.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/mem_encrypt.S | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Sean Christopherson March 10, 2021, 4:08 p.m. UTC | #1 
On Wed, Mar 10, 2021, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The 32-bit #VC handler has no GHCB and can only handle CPUID exit codes.
> It is needed by the early boot code to handle #VC exceptions raised in
> verify_cpu() and to get the position of the C bit.
> 
> But the CPUID information comes from the hypervisor, which is untrusted
> and might return results which trick the guest into the no-SEV boot path
> with no C bit set in the page-tables. All data written to memory would
> then be unencrypted and could leak sensitive data to the hypervisor.
> 
> Add sanity checks to the 32-bit boot #VC handler to make sure the
> hypervisor does not pretend that SEV is not enabled.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/boot/compressed/mem_encrypt.S | 36 ++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> index 2ca056a3707c..8941c3a8ff8a 100644
> --- a/arch/x86/boot/compressed/mem_encrypt.S
> +++ b/arch/x86/boot/compressed/mem_encrypt.S
> @@ -145,6 +145,34 @@ SYM_CODE_START(startup32_vc_handler)
>  	jnz	.Lfail
>  	movl	%edx, 0(%esp)		# Store result
>  
> +	/*
> +	 * Sanity check CPUID results from the Hypervisor. See comment in
> +	 * do_vc_no_ghcb() for more details on why this is necessary.
> +	 */
> +
> +	/* Fail if Hypervisor bit not set in CPUID[1].ECX[31] */

This check is flawed, as is the existing check in 64-bit boot.  Or I guess more
accurately, the check in get_sev_encryption_bit() is flawed.  AIUI, SEV-ES
doesn't require the hypervisor to intercept CPUID.  A malicious hypervisor can
temporarily pass-through CPUID to bypass the CPUID[1].ECX[31] check.  The
hypervisor likely has access to the guest firmware source, so it wouldn't be
difficult for the hypervisor to disable CPUID interception once it detects that
firmware is handing over control to the kernel.

> +	cmpl    $1, %ebx
> +	jne     .Lcheck_leaf
> +	btl     $31, 4(%esp)
> +	jnc     .Lfail
> +	jmp     .Ldone
> +
> +.Lcheck_leaf:
> +	/* Fail if SEV leaf not available in CPUID[0x80000000].EAX */
> +	cmpl    $0x80000000, %ebx
> +	jne     .Lcheck_sev
> +	cmpl    $0x8000001f, 12(%esp)
> +	jb      .Lfail
> +	jmp     .Ldone
> +
> +.Lcheck_sev:
> +	/* Fail if SEV bit not set in CPUID[0x8000001f].EAX[1] */
> +	cmpl    $0x8000001f, %ebx
> +	jne     .Ldone
> +	btl     $1, 12(%esp)
> +	jnc     .Lfail
> +
> +.Ldone:
>  	popl	%edx
>  	popl	%ecx
>  	popl	%ebx
> @@ -158,6 +186,14 @@ SYM_CODE_START(startup32_vc_handler)
>  
>  	iret
>  .Lfail:
> +	/* Send terminate request to Hypervisor */
> +	movl    $0x100, %eax
> +	xorl    %edx, %edx
> +	movl    $MSR_AMD64_SEV_ES_GHCB, %ecx
> +	wrmsr
> +	rep; vmmcall
> +
> +	/* If request fails, go to hlt loop */
>  	hlt
>  	jmp .Lfail
>  SYM_CODE_END(startup32_vc_handler)
> -- 
> 2.30.1
>
Martin Radev March 10, 2021, 5:26 p.m. UTC | #2 
On Wed, Mar 10, 2021 at 08:08:37AM -0800, Sean Christopherson wrote:
> On Wed, Mar 10, 2021, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel@suse.de>
> > 
> > The 32-bit #VC handler has no GHCB and can only handle CPUID exit codes.
> > It is needed by the early boot code to handle #VC exceptions raised in
> > verify_cpu() and to get the position of the C bit.
> > 
> > But the CPUID information comes from the hypervisor, which is untrusted
> > and might return results which trick the guest into the no-SEV boot path
> > with no C bit set in the page-tables. All data written to memory would
> > then be unencrypted and could leak sensitive data to the hypervisor.
> > 
> > Add sanity checks to the 32-bit boot #VC handler to make sure the
> > hypervisor does not pretend that SEV is not enabled.
> > 
> > Signed-off-by: Joerg Roedel <jroedel@suse.de>
> > ---
> >  arch/x86/boot/compressed/mem_encrypt.S | 36 ++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> > index 2ca056a3707c..8941c3a8ff8a 100644
> > --- a/arch/x86/boot/compressed/mem_encrypt.S
> > +++ b/arch/x86/boot/compressed/mem_encrypt.S
> > @@ -145,6 +145,34 @@ SYM_CODE_START(startup32_vc_handler)
> >  	jnz	.Lfail
> >  	movl	%edx, 0(%esp)		# Store result
> >  
> > +	/*
> > +	 * Sanity check CPUID results from the Hypervisor. See comment in
> > +	 * do_vc_no_ghcb() for more details on why this is necessary.
> > +	 */
> > +
> > +	/* Fail if Hypervisor bit not set in CPUID[1].ECX[31] */
> 
> This check is flawed, as is the existing check in 64-bit boot.  Or I guess more
> accurately, the check in get_sev_encryption_bit() is flawed.  AIUI, SEV-ES
> doesn't require the hypervisor to intercept CPUID.  A malicious hypervisor can
> temporarily pass-through CPUID to bypass the CPUID[1].ECX[31] check.

If erroneous information is provided, either through interception or without, there's
this check which is performed every time a new page table is set in the early linux stages:
https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/x86/kernel/sev_verify_cbit.S#L22

This should lead to a halt if corruption is detected, unless I'm overlooking something.
Please share more info.


> The
> hypervisor likely has access to the guest firmware source, so it wouldn't be
> difficult for the hypervisor to disable CPUID interception once it detects that
> firmware is handing over control to the kernel.
> 

You probably don't even need to know the firmware for that. There the option to set CR* changes to cause
#AE which probably gives away enough information.

> > +	cmpl    $1, %ebx
> > +	jne     .Lcheck_leaf
> > +	btl     $31, 4(%esp)
> > +	jnc     .Lfail
> > +	jmp     .Ldone
> > +
> > +.Lcheck_leaf:
> > +	/* Fail if SEV leaf not available in CPUID[0x80000000].EAX */
> > +	cmpl    $0x80000000, %ebx
> > +	jne     .Lcheck_sev
> > +	cmpl    $0x8000001f, 12(%esp)
> > +	jb      .Lfail
> > +	jmp     .Ldone
> > +
> > +.Lcheck_sev:
> > +	/* Fail if SEV bit not set in CPUID[0x8000001f].EAX[1] */
> > +	cmpl    $0x8000001f, %ebx
> > +	jne     .Ldone
> > +	btl     $1, 12(%esp)
> > +	jnc     .Lfail
> > +
> > +.Ldone:
> >  	popl	%edx
> >  	popl	%ecx
> >  	popl	%ebx
> > @@ -158,6 +186,14 @@ SYM_CODE_START(startup32_vc_handler)
> >  
> >  	iret
> >  .Lfail:
> > +	/* Send terminate request to Hypervisor */
> > +	movl    $0x100, %eax
> > +	xorl    %edx, %edx
> > +	movl    $MSR_AMD64_SEV_ES_GHCB, %ecx
> > +	wrmsr
> > +	rep; vmmcall
> > +
> > +	/* If request fails, go to hlt loop */
> >  	hlt
> >  	jmp .Lfail
> >  SYM_CODE_END(startup32_vc_handler)
> > -- 
> > 2.30.1
> >
Sean Christopherson March 10, 2021, 5:51 p.m. UTC | #3 
On Wed, Mar 10, 2021, Martin Radev wrote:
> On Wed, Mar 10, 2021 at 08:08:37AM -0800, Sean Christopherson wrote:
> > On Wed, Mar 10, 2021, Joerg Roedel wrote:
> > > +	/*
> > > +	 * Sanity check CPUID results from the Hypervisor. See comment in
> > > +	 * do_vc_no_ghcb() for more details on why this is necessary.
> > > +	 */
> > > +
> > > +	/* Fail if Hypervisor bit not set in CPUID[1].ECX[31] */
> > 
> > This check is flawed, as is the existing check in 64-bit boot.  Or I guess more
> > accurately, the check in get_sev_encryption_bit() is flawed.  AIUI, SEV-ES
> > doesn't require the hypervisor to intercept CPUID.  A malicious hypervisor can
> > temporarily pass-through CPUID to bypass the CPUID[1].ECX[31] check.
> 
> If erroneous information is provided, either through interception or without, there's
> this check which is performed every time a new page table is set in the early linux stages:
> https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/x86/kernel/sev_verify_cbit.S#L22
> 
> This should lead to a halt if corruption is detected, unless I'm overlooking something.
> Please share more info.

That check is predicated on sme_me_mask != 0, sme_me_mask is set based on the
result of get_sev_encryption_bit(), and that returns '0' if CPUID[1].ECX[31] is
'0'.

sme_enable() also appears to have the same issue, as CPUID[1].ECX[31]=0 would
cause it to check for SME instead of SEV, and the hypervisor can simply return
0 for a VMGEXIT to get MSR_K8_SYSCFG.

I've no idea if the guest would actually survive with a bogus sme_me_mask, but
relying on CPUID[1] to #VC is flawed.

Since MSR_AMD64_SEV is non-interceptable, that seems like it should be the
canonical way to detect SEV/SEV-ES.  The only complication seems to be handling
#GP faults on the RDMSR in early boot.

> > The hypervisor likely has access to the guest firmware source, so it
> > wouldn't be difficult for the hypervisor to disable CPUID interception once
> > it detects that firmware is handing over control to the kernel.
> > 
> 
> You probably don't even need to know the firmware for that. There the option
> to set CR* changes to cause #AE which probably gives away enough information.
Martin Radev March 10, 2021, 6:10 p.m. UTC | #4 
On Wed, Mar 10, 2021 at 09:51:48AM -0800, Sean Christopherson wrote:
> On Wed, Mar 10, 2021, Martin Radev wrote:
> > On Wed, Mar 10, 2021 at 08:08:37AM -0800, Sean Christopherson wrote:
> > > On Wed, Mar 10, 2021, Joerg Roedel wrote:
> > > > +	/*
> > > > +	 * Sanity check CPUID results from the Hypervisor. See comment in
> > > > +	 * do_vc_no_ghcb() for more details on why this is necessary.
> > > > +	 */
> > > > +
> > > > +	/* Fail if Hypervisor bit not set in CPUID[1].ECX[31] */
> > > 
> > > This check is flawed, as is the existing check in 64-bit boot.  Or I guess more
> > > accurately, the check in get_sev_encryption_bit() is flawed.  AIUI, SEV-ES
> > > doesn't require the hypervisor to intercept CPUID.  A malicious hypervisor can
> > > temporarily pass-through CPUID to bypass the CPUID[1].ECX[31] check.
> > 
> > If erroneous information is provided, either through interception or without, there's
> > this check which is performed every time a new page table is set in the early linux stages:
> > https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/x86/kernel/sev_verify_cbit.S#L22
> > 
> > This should lead to a halt if corruption is detected, unless I'm overlooking something.
> > Please share more info.
> 
> That check is predicated on sme_me_mask != 0, sme_me_mask is set based on the
> result of get_sev_encryption_bit(), and that returns '0' if CPUID[1].ECX[31] is
> '0'.
> 
> sme_enable() also appears to have the same issue, as CPUID[1].ECX[31]=0 would
> cause it to check for SME instead of SEV, and the hypervisor can simply return
> 0 for a VMGEXIT to get MSR_K8_SYSCFG.
> 
> I've no idea if the guest would actually survive with a bogus sme_me_mask, but
> relying on CPUID[1] to #VC is flawed.
> 
> Since MSR_AMD64_SEV is non-interceptable, that seems like it should be the
> canonical way to detect SEV/SEV-ES.  The only complication seems to be handling
> #GP faults on the RDMSR in early boot.
> 
> > > The hypervisor likely has access to the guest firmware source, so it
> > > wouldn't be difficult for the hypervisor to disable CPUID interception once
> > > it detects that firmware is handing over control to the kernel.
> > > 
> > 
> > You probably don't even need to know the firmware for that. There the option
> > to set CR* changes to cause #AE which probably gives away enough information.

I see what you mean but I never tried out disabling interception for cpuid.
There was the idea of checking for bogus information in the VC handler, but what
you suggested would bypass it, I guess.

If the C-bit is not set and memory gets interpreted as unencrypted, then the HV
can gain code execution easily by means of ROP and then switch to the OVMF page
table to easily do proper payload injection.

If interested, check video at https://fosdem.org/2021/schedule/event/tee_sev_es/
on minute 15.
diff mbox series

Patch

diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 2ca056a3707c..8941c3a8ff8a 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -145,6 +145,34 @@  SYM_CODE_START(startup32_vc_handler)
 	jnz	.Lfail
 	movl	%edx, 0(%esp)		# Store result
 
+	/*
+	 * Sanity check CPUID results from the Hypervisor. See comment in
+	 * do_vc_no_ghcb() for more details on why this is necessary.
+	 */
+
+	/* Fail if Hypervisor bit not set in CPUID[1].ECX[31] */
+	cmpl    $1, %ebx
+	jne     .Lcheck_leaf
+	btl     $31, 4(%esp)
+	jnc     .Lfail
+	jmp     .Ldone
+
+.Lcheck_leaf:
+	/* Fail if SEV leaf not available in CPUID[0x80000000].EAX */
+	cmpl    $0x80000000, %ebx
+	jne     .Lcheck_sev
+	cmpl    $0x8000001f, 12(%esp)
+	jb      .Lfail
+	jmp     .Ldone
+
+.Lcheck_sev:
+	/* Fail if SEV bit not set in CPUID[0x8000001f].EAX[1] */
+	cmpl    $0x8000001f, %ebx
+	jne     .Ldone
+	btl     $1, 12(%esp)
+	jnc     .Lfail
+
+.Ldone:
 	popl	%edx
 	popl	%ecx
 	popl	%ebx
@@ -158,6 +186,14 @@  SYM_CODE_START(startup32_vc_handler)
 
 	iret
 .Lfail:
+	/* Send terminate request to Hypervisor */
+	movl    $0x100, %eax
+	xorl    %edx, %edx
+	movl    $MSR_AMD64_SEV_ES_GHCB, %ecx
+	wrmsr
+	rep; vmmcall
+
+	/* If request fails, go to hlt loop */
 	hlt
 	jmp .Lfail
 SYM_CODE_END(startup32_vc_handler)