Re: [edk2-devel] [PATCH 03/12] OvmfPkg/ResetVector: Validate the encryption bit position for SEV/SEV-ES

["Laszlo Ersek" <lersek@redhat.com>]

On 12/15/20 21:51, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3108
> 
> To help mitigate against ROP attacks, add some checks to validate the
> encryption bit position that is reported by the hypervisor.
> 
> The first check is to ensure that the hypervisor reports a bit position
> above bit 31. After extracting the encryption bit position from the CPUID
> information, the code checks that the value is above 31. If the value is
> not above 31, then the bit position is not valid, so the code enters a
> HLT loop.
> 
> The second check is specific to SEV-ES guests and is a two step process.
> The first step will obtain random data using RDRAND and store that data to
> memory before paging is enabled. When paging is not enabled, all writes to
> memory are encrypted. The random data is maintained in registers, which
> are protected. After enabling paging, the random data in memory is
> compared to the register contents. If they don't match, then the reported
> bit position is not valid, so the code enters a HLT loop.

(1) Please replace:

  After enabling paging,

with:

  The second step is that, after enabling paging,

> 
> The third check is after switching to 64-bit long mode. Use the fact that
> instruction fetches are automatically decrypted, while a memory fetch is
> decrypted only if the encryption bit is set in the page table. By
> comparing the bytes of an instruction fetch against a memory read of that
> same instruction, the encryption bit position can be validated. If the
> compare is not equal, then SEV/SEV-ES is active but the reported bit
> position is not valid, so the code enters a HLT loop.

I had to stare quite long at the commit message and the code, but
ultimately, it is clearly documented that the 1st and 3rd checks cover
both SEV and SEV-ES, while the 2nd check only covers SEV-ES. OK.

> 
> The encryption mask is saved in the SEV-ES work area so that it can be
> used later in the boot process.

(2) This does not seem to happen in this patch.

If you agree, please drop this paragraph from the commit message.

> 
> To keep the changes local to the OvmfPkg, an OvmfPkg version of the
> Flat32ToFlat64.asm file has been created based on the UefiCpuPkg file
> UefiCpuPkg/ResetVector/Vtf0/Ia32/Flat32ToFlat64.asm.

Thanks for this hint. Reviewing this patch with "--find-copies-harder
-C20" is indeed easier.

> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Include/Library/MemEncryptSevLib.h  |   4 +
>  OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 116 ++++++++++++++++++++
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm   |  12 +-
>  OvmfPkg/ResetVector/ResetVector.nasmb       |   4 +-
>  4 files changed, 133 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index a6d82dac7fac..dc09c61e58bb 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -21,10 +21,14 @@
>  // This structure is also used by assembler files:
>  //   OvmfPkg/ResetVector/ResetVector.nasmb
>  //   OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +//   OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
>  // any changes must stay in sync with its usage.
>  //
>  typedef struct _SEC_SEV_ES_WORK_AREA {
>    UINT8    SevEsEnabled;
> +  UINT8    Reserved1[7];
> +
> +  UINT64   RandomData;
>  } SEC_SEV_ES_WORK_AREA;
>  
>  /**
> diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
> new file mode 100644
> index 000000000000..8fe0d0eed945
> --- /dev/null
> +++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
> @@ -0,0 +1,116 @@
> +;------------------------------------------------------------------------------
> +; @file
> +; Transition from 32 bit flat protected mode into 64 bit flat protected mode
> +;
> +; Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +;------------------------------------------------------------------------------
> +
> +BITS    32
> +
> +;
> +; Modified:  EAX, ECX, EDX
> +;
> +Transition32FlatTo64Flat:
> +
> +    OneTimeCall SetCr3ForPageTables64
> +
> +    mov     eax, cr4
> +    bts     eax, 5                      ; enable PAE
> +    mov     cr4, eax
> +
> +    mov     ecx, 0xc0000080
> +    rdmsr
> +    bts     eax, 8                      ; set LME
> +    wrmsr
> +
> +    ;
> +    ; SEV-ES mitigation check support
> +    ;
> +    xor     ebx, ebx
> +
> +    cmp     byte[SEV_ES_WORK_AREA], 0
> +    jz      EnablePaging
> +
> +    ;
> +    ; SEV-ES is active, perform a quick sanity check against the reported
> +    ; encryption bit position. This is to help mitigate against attacks where
> +    ; the hypervisor reports an incorrect encryption bit position.
> +    ;
> +    ; This is the first step in a two step process. Before paging is enabled
> +    ; writes to memory are encrypted. Using the RDRAND instruction (available
> +    ; on all SEV capable processors), write 64-bits of random data to the
> +    ; SEV_ES_WORK_AREA and maintain the random data in registers (register
> +    ; state is protected under SEV-ES). This will be used in the second step.
> +    ;
> +RdRand1:
> +    rdrand  ecx
> +    jnc     RdRand1
> +    mov     dword[SEV_ES_WORK_AREA_RDRAND], ecx
> +RdRand2:
> +    rdrand  edx
> +    jnc     RdRand2
> +    mov     dword[SEV_ES_WORK_AREA_RDRAND + 4], edx
> +
> +    ;
> +    ; Use EBX instead of the SEV_ES_WORK_AREA memory to determine whether to
> +    ; perform the second step.
> +    ;
> +    mov     ebx, 1
> +
> +EnablePaging:
> +    mov     eax, cr0
> +    bts     eax, 31                     ; set PG
> +    mov     cr0, eax                    ; enable paging
> +
> +    jmp     LINEAR_CODE64_SEL:ADDR_OF(jumpTo64BitAndLandHere)
> +BITS    64
> +jumpTo64BitAndLandHere:
> +
> +    ;
> +    ; Check if the second step of the SEV-ES

(3) Please finish this comment.

> +    test    ebx, ebx
> +    jz      InsnCompare
> +
> +    ;
> +    ; SEV-ES is active, perform the second step of the encryption bit postion
> +    ; mitigation check. The ECX and EDX register contain data from RDRAND that
> +    ; was stored to memory in encrypted form. If the encryption bit position is
> +    ; valid, the contents of ECX and EDX will match the memory location.
> +    ;
> +    cmp     dword[SEV_ES_WORK_AREA_RDRAND], ecx
> +    jne     SevEncBitHlt
> +    cmp     dword[SEV_ES_WORK_AREA_RDRAND + 4], edx
> +    jne     SevEncBitHlt
> +
> +    ;
> +    ; If SEV or SEV-ES is active, perform a quick sanity check against
> +    ; the reported encryption bit position. This is to help mitigate
> +    ; against attacks where the hypervisor reports an incorrect encryption
> +    ; bit position. If SEV is not active, this check will always succeed.
> +    ;
> +    ; The cmp instruction compares the first four bytes of the cmp instruction
> +    ; itself (which will be read decrypted if SEV or SEV-ES is active and the
> +    ; encryption bit position is valid) against the immediate within the
> +    ; instruction (an instruction fetch is always decrypted correctly by
> +    ; hardware) based on RIP relative addressing.
> +    ;
> +InsnCompare:
> +    cmp     dword[rel InsnCompare], 0xFFF63D81
> +    je      GoodCompare
> +
> +    ;
> +    ; The hypervisor provided an incorrect encryption bit position, do not
> +    ; proceed.
> +    ;
> +SevEncBitHlt:
> +    hlt
> +    jmp     SevEncBitHlt
> +

(4) Considering *both* HLT loops introduced in this patch:

would it make sense to insert a CLI before *each* HLT?

In UefiCpuPkg, we do that in several places.

I'm guessing it might help if the hypervisor tried to inject #VC or some
other exception while the guest is intentionally stuck in the HLT loop.
(I don't know if forcing the guest to run an exception handler is in any
way exploitable, I just think once we land here, the hypervisor should
have as little control as possible.)

The patch looks fine otherwise.

Thanks
Laszlo

> +GoodCompare:
> +    debugShowPostCode POSTCODE_64BIT_MODE
> +
> +    OneTimeCallRet Transition32FlatTo64Flat
> +
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 4032719c3075..3cd909df4f09 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -140,9 +140,17 @@ GetSevEncBit:
>      ; Get pte bit position to enable memory encryption
>      ; CPUID Fn8000_001F[EBX] - Bits 5:0
>      ;
> +    and       ebx, 0x3f
>      mov       eax, ebx
> -    and       eax, 0x3f
> -    jmp       SevExit
> +
> +    ; The encryption bit position is always above 31
> +    sub       ebx, 32
> +    jns       SevExit
> +
> +    ; Encryption bit was reported as 31 or below, enter a HLT loop
> +SevEncBitLowHlt:
> +    hlt
> +    jmp       SevEncBitLowHlt
>  
>  NoSev:
>      xor       eax, eax
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index c5e0fe93abf4..d3aa87982959 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -3,6 +3,7 @@
>  ; This file includes all other code files to assemble the reset vector code
>  ;
>  ; Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
>  ;
>  ;------------------------------------------------------------------------------
> @@ -67,13 +68,14 @@
>    %endif
>  
>    %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
> -%include "Ia32/Flat32ToFlat64.asm"
>  
>    %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>    %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>    %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>    %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
> +  %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
>    %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
> +%include "Ia32/Flat32ToFlat64.asm"
>  %include "Ia32/PageTables64.asm"
>  %endif
>  
>