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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 1/4/21 1:59 PM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C1fc5692b60664b1323db08d8b0eb372d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637453871588219864%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=J4HNck3XxUEfW1exEoa52sp6p3EliBd2jgqDJT%2BqYa4%3D&amp;reserved=0
>>
>> 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,

Will do.

> 
>>
>> 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.

Yes, will do. A left over comment from what is now done in a later patch.

> 
>>
>> 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.

Will do.

> 
>> +    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.)
> 

That makes sense. I'll add a CLI before each HLT.

Thanks,
Tom

> 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
>>   
>>
>