[edk2-devel] [PATCH v7 06/31] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase

Brijesh Singh via groups.io posted 31 patches 4 years, 5 months ago
There is a newer version of this series
[edk2-devel] [PATCH v7 06/31] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
Posted by Brijesh Singh via groups.io 4 years, 5 months ago
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

An SEV-SNP guest requires that private memory (aka pages mapped encrypted)
must be validated before being accessed.

The validation process consist of the following sequence:

1) Set the memory encryption attribute in the page table (aka C-bit).
   Note: If the processor is in non-PAE mode, then all the memory accesses
   are considered private.
2) Add the memory range as private in the RMP table. This can be performed
   using the Page State Change VMGEXIT defined in the GHCB specification.
3) Use the PVALIDATE instruction to set the Validated Bit in the RMP table.

During the guest creation time, the VMM encrypts the OVMF_CODE.fd using
the SEV-SNP firmware provided LAUNCH_UPDATE_DATA command. In addition to
encrypting the content, the command also validates the memory region.
This allows us to execute the code without going through the validation
sequence.

During execution, the reset vector need to access some data pages
(such as page tables, SevESWorkarea, Sec stack). The data pages are
accessed as private memory. The data pages are not part of the
OVMF_CODE.fd, so they were not validated during the guest creation.

There are two approaches we can take to validate the data pages before
the access:

a) Enhance the OVMF reset vector code to validate the pages as described
   above (go through step 2 - 3).
OR
b) Validate the pages during the guest creation time. The SEV firmware
   provides a command which can be used by the VMM to validate the pages
   without affecting the measurement of the launch.

Approach #b seems much simpler; it does not require any changes to the
OVMF reset vector code.

Update the OVMF metadata with the list of regions that must be
pre-validated by the VMM before the boot.

Cc: Michael Roth <michael.roth@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/ResetVector/ResetVector.inf      |  7 ++++
 OvmfPkg/ResetVector/ResetVector.nasmb    | 29 ++++++++++++++
 OvmfPkg/ResetVector/X64/OvmfMetadata.asm | 48 ++++++++++++++++++++++++
 3 files changed, 84 insertions(+)

diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index 4cb81a3233f0..af3cf610a2cd 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -37,6 +37,8 @@ [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
@@ -44,6 +46,11 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
+  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
+  gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
 [FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 84cb5ae81b66..e05e202def1e 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -70,6 +70,7 @@
   %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
 
   %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
+  %define GHCB_PT_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbPageTableSize))
   %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
   %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
   %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
@@ -81,6 +82,34 @@
   %define SEV_SNP_SECRETS_SIZE  (FixedPcdGet32 (PcdOvmfSnpSecretsSize))
   %define CPUID_BASE  (FixedPcdGet32 (PcdOvmfCpuidBase))
   %define CPUID_SIZE  (FixedPcdGet32 (PcdOvmfCpuidSize))
+  %define SEC_PAGE_TABLE_BASE   (FixedPcdGet32 (PcdOvmfSecPageTablesBase))
+  %define SEC_PAGE_TABLE_SIZE   (FixedPcdGet32 (PcdOvmfSecPageTablesSize))
+  %define LOCK_BOX_STORAGE_BASE (FixedPcdGet32 (PcdOvmfLockBoxStorageBase))
+  %define LOCK_BOX_STORAGE_SIZE (FixedPcdGet32 (PcdOvmfLockBoxStorageSize))
+  ;
+  ; The PcdGuidedExtractHandlerTableAddress is a 64-bit PCD. The FixedPcdGet64() returns
+  ; a number suffix with 'ULL' (e.g 0x1111ULL). NASM does not like the constant ending
+  ; with anything other than 'h'. So, instead of using the FixedPcdGet64(), calculate
+  ; the base address of GuidedExtractHandlerTableBase
+  ;
+  %define GUID_EXTRACT_HANDLER_TABLE_BASE (LOCK_BOX_STORAGE_BASE + LOCK_BOX_STORAGE_SIZE)
+  %define GUID_EXTRACT_HANDLER_TABLE_SIZE (FixedPcdGet32 (PcdGuidedExtractHandlerTableSize))
+  %define WORK_AREA_BASE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
+  %define WORK_AREA_SIZE (FixedPcdGet32 (PcdOvmfWorkAreaSize))
+  %define SEC_PEI_TEMP_RAM_BASE (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase))
+  %define SEC_PEI_TEMP_RAM_SIZE (FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
+
+  ;
+  ; The PcdOvmfSecGhcbBase reserves two GHCB pages. The first page is used
+  ; as GHCB shared page and second is used for booking to support the
+  ; nested GHCB in SEC phase. The booking page is mapped private. The VMM
+  ; does not need to validate the shared page but it need to validate the
+  ; booking page
+  ;
+  %define GHCB_SEC_BACKUP_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBackupBase))
+  %define GHCB_SEC_BACKUP_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbBackupSize))
+  %define GHCB_BOOKKEEPING_BASE (GHCB_BASE + 0x1000)
+  %define GHCB_BOOKKEEPING_SIZE (GHCB_SIZE - 0x1000)
 
 %include "Ia32/Flat32ToFlat64.asm"
 %include "Ia32/AmdSev.asm"
diff --git a/OvmfPkg/ResetVector/X64/OvmfMetadata.asm b/OvmfPkg/ResetVector/X64/OvmfMetadata.asm
index 95bac86a3b95..657d6c298d4e 100644
--- a/OvmfPkg/ResetVector/X64/OvmfMetadata.asm
+++ b/OvmfPkg/ResetVector/X64/OvmfMetadata.asm
@@ -49,6 +49,48 @@ _Descriptor:
   DD OVMF_METADATA_VERSION                            ; Version
   DD (OvmfGuidedStructureEnd - _Descriptor - 16) / 12 ; Number of sections
 
+; Page table used during SEC
+SecPageTable:
+  DD  SEC_PAGE_TABLE_BASE
+  DD  SEC_PAGE_TABLE_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
+; Lockbox storage
+LockBoxStorage:
+  DD  LOCK_BOX_STORAGE_BASE
+  DD  LOCK_BOX_STORAGE_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
+; Guided Extract Handler Table
+ExtractHandlerTable:
+  DD  GUID_EXTRACT_HANDLER_TABLE_BASE
+  DD  GUID_EXTRACT_HANDLER_TABLE_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
+; GHCB page table
+GhcbPageTable:
+  DD  GHCB_PT_ADDR
+  DD  GHCB_PT_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
+; GHCB bookkeeping page used in SEC phase
+GhcbBookkeeping:
+  DD  GHCB_BOOKKEEPING_BASE
+  DD  GHCB_BOOKKEEPING_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
+; Confidential computing work area
+WorkArea:
+  DD  WORK_AREA_BASE
+  DD  WORK_AREA_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
+; GHCB backup page used in SEC
+GhcbBackup:
+  DD  GHCB_SEC_BACKUP_BASE
+  DD  GHCB_SEC_BACKUP_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
 ; SEV-SNP Secrets page
 SevSnpSecrets:
   DD  SEV_SNP_SECRETS_BASE
@@ -61,5 +103,11 @@ CpuidSec:
   DD  CPUID_SIZE
   DD  OVMF_SECTION_TYPE_CPUID
 
+; Temporary RAM used in SEC phase
+SecPeiTempRam:
+  DD  SEC_PEI_TEMP_RAM_BASE
+  DD  SEC_PEI_TEMP_RAM_SIZE
+  DD  OVMF_SECTION_TYPE_SEC_MEM
+
 OvmfGuidedStructureEnd:
   ALIGN   16
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80586): https://edk2.groups.io/g/devel/message/80586
Mute This Topic: https://groups.io/mt/85582693/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v7 06/31] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
Posted by Gerd Hoffmann 4 years, 4 months ago
  Hi,

> +; GHCB page table
> +GhcbPageTable:
> +  DD  GHCB_PT_ADDR
> +  DD  GHCB_PT_SIZE
> +  DD  OVMF_SECTION_TYPE_SEC_MEM

Hmm, TDX will re-use those pages for something else.  So as long as TDX
is happy with OVMF_SECTION_TYPE_SEC_MEM everything should work fine and
just mentioning that in the comments will be ok.

If not we might need a SEV_SECTION_TYPE_SEC_MEM to indicate the entry is
valid for SEV only and Intel can add a separate
TDX_SECTION_TYPE_SOMETHING_ELSE entry for the same address range.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80759): https://edk2.groups.io/g/devel/message/80759
Mute This Topic: https://groups.io/mt/85582693/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v7 06/31] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
Posted by Brijesh Singh via groups.io 4 years, 4 months ago
On 9/16/21 3:26 AM, Gerd Hoffmann via groups.io wrote:
>   Hi,
>
>> +; GHCB page table
>> +GhcbPageTable:
>> +  DD  GHCB_PT_ADDR
>> +  DD  GHCB_PT_SIZE
>> +  DD  OVMF_SECTION_TYPE_SEC_MEM
> Hmm, TDX will re-use those pages for something else.  So as long as TDX
> is happy with OVMF_SECTION_TYPE_SEC_MEM everything should work fine and
> just mentioning that in the comments will be ok.
>
> If not we might need a SEV_SECTION_TYPE_SEC_MEM to indicate the entry is
> valid for SEV only and Intel can add a separate
> TDX_SECTION_TYPE_SOMETHING_ELSE entry for the same address range.

Yep, looking at the current TDX patches we see that GHCB is used for the
mailbox,  to make integration easy its good idea that we define
SEV_SECTION_TYPE_SEC_MEM and use it for those GHCB memory pages. I will
add in next version.

thanks



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80777): https://edk2.groups.io/g/devel/message/80777
Mute This Topic: https://groups.io/mt/85582693/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-