lkml.org 
[lkml]   [2021]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/7] x86/boot/compressed/64: Add CPUID sanity check to 32-bit boot-path
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
> >

\
 
 \ /
  Last update: 2021-03-10 18:29    [W:0.052 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site