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

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: <YEkBU9em9SQZ25vA@martin-ThinkPad-T440p>
Date:   Wed, 10 Mar 2021 18:26:43 +0100
From:   Martin Radev <martin.b.radev@...il.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Joerg Roedel <joro@...tes.org>, x86@...nel.org,
        Joerg Roedel <jroedel@...e.de>, hpa@...or.com,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Jiri Slaby <jslaby@...e.cz>,
        Dan Williams <dan.j.williams@...el.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Juergen Gross <jgross@...e.com>,
        Kees Cook <keescook@...omium.org>,
        David Rientjes <rientjes@...gle.com>,
        Cfir Cohen <cfir@...gle.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mike Stunes <mstunes@...are.com>,
        Arvind Sankar <nivedita@...m.mit.edu>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [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@...e.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@...e.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
> >