On Wed, Mar 10, 2021 at 08:08:37AM -0800, Sean Christopherson wrote: > On Wed, Mar 10, 2021, Joerg Roedel wrote: > > From: Joerg Roedel <[email protected]> > > > > 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 <[email protected]> > > --- > > 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 > >
