[edk2-devel] [PATCH 03/12] OvmfPkg/ResetVector: Validate the encryption bit position for SEV/SEV-ES
Laszlo Ersek
lersek at redhat.com
Mon Jan 4 19:59:06 UTC 2021
On 12/15/20 21:51, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky at 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 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.
> + 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
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69630): https://edk2.groups.io/g/devel/message/69630
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