Re: [PATCH v3 2/5] x86/boot/compressed/64: Add CPUID sanity check to early #VC handler

[Borislav Petkov]

On Wed, Oct 21, 2020 at 02:39:35PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@xxxxxxx>
> 
> The early #VC handler which doesn't have a GHCB 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 early #VC handlers to make sure the hypervisor
> can not pretend that SEV is disabled.
> 
> Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
> ---
>  arch/x86/kernel/sev-es-shared.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/x86/kernel/sev-es-shared.c b/arch/x86/kernel/sev-es-shared.c
> index 5f83ccaab877..48bb14563dcd 100644
> --- a/arch/x86/kernel/sev-es-shared.c
> +++ b/arch/x86/kernel/sev-es-shared.c
> @@ -178,6 +178,32 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>  		goto fail;
>  	regs->dx = val >> 32;
>  
> +	/*
> +	 * This is a VC handler and it is only raised when SEV-ES is active,

				and the #VC is only raised...

> +	 * which means SEV must be active too. Do sanity checks on the CPUID
> +	 * results to make sure the hypervisor does not trick the kernel into
> +	 * the no-sev path. This could map sensitive data unencrypted and make
> +	 * it accessible to the hypervisor.
> +	 *
> +	 * In particular, check for:
> +	 *	- Hypervisor CPUID bit
> +	 *	- Availability of CPUID leaf 0x8000001f
> +	 *	- SEV CPUID bit.
> +	 *
> +	 * The hypervisor might still report the wrong C-bit position, but this
> +	 * can't be checked here.

So why are we doing those checks here at all then? I mean, the HV
can tell us whatever it wants, i.e., make sure those checks pass but
still report the C-bit at the wrong position. Which means that those
checks are simply meh. So why are we doing them at all? To catch stupid
hypervisors who can't even lie properly to the guest? :-)

> +	 */
> +
> +	if ((fn == 1 && !(regs->cx & BIT(31))))
> +		/* Hypervisor Bit */

s/Bit/bit/g

> +		goto fail;
> +	else if (fn == 0x80000000 && (regs->ax < 0x8000001f))
> +		/* SEV Leaf check */
> +		goto fail;
> +	else if ((fn == 0x8000001f && !(regs->ax & BIT(1))))
> +		/* SEV Bit */
> +		goto fail;
> +
>  	/* Skip over the CPUID two-byte opcode */
>  	regs->ip += 2;
>  
> -- 
> 2.28.0
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette