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.
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.
The encryption mask is saved in the SEV-ES work area so that it can be
used later in the boot process.
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.
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
+ 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
+
+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
--
2.28.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68903): https://edk2.groups.io/g/devel/message/68903
Mute This Topic: https://groups.io/mt/78986151/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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&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@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
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.