[edk2-devel] [PATCH 03/12] OvmfPkg/ResetVector: Validate the encryption bit position for SEV/SEV-ES
Lendacky, Thomas
thomas.lendacky at amd.com
Mon Jan 4 20:45:31 UTC 2021
On 1/4/21 1:59 PM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky at amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&data=04%7C01%7Cthomas.lendacky%40amd.com%7C1fc5692b60664b1323db08d8b0eb372d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637453871588219864%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J4HNck3XxUEfW1exEoa52sp6p3EliBd2jgqDJT%2BqYa4%3D&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 at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69633): https://edk2.groups.io/g/devel/message/69633
Mute This Topic: https://groups.io/mt/78986151/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list