[PATCH 0/2] OvmfPkg: Enable measured direct boot on AMD SEV-SNP

[PATCH 0/2] OvmfPkg: Enable measured direct boot on AMD SEV-SNP

[Dov Murik]

AMD SEV and SEV-ES support measured direct boot with
kernel/initrd/cmdline hashes injected by QEMU and verified by OVMF
during boot.

To enable the same approach for AMD SEV-SNP we make sure the page in
which QEMU inserts the hashes of kernel/initrd/cmdline is not already
pre-validated, as SNP doesn't allow validating a page twice.

The first patch rearranges the pages in AmdSevX64's MEMFD so they are in
the same order both as in the main target (OvmfPkgX64), with the
exception of the SEV Launch Secret page which isn't defined in
OvmfPkgX64.

The second patch modifies the SNP metadata structure such that on
AmdSev target the SEV Launch Secret page is not included in the ranges
that are pre-validated (zero pages) by the VMM; instead the VMM will
insert content into this page, or mark it explicitly as a zero page if
no hashes are added.

A corresponding RFC patch to QEMU will be published soon.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>

Dov Murik (2):
  OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in
    OvmfPkgX64.fdf
  OvmfPkg/ResetVector: Exclude SEV launch secrets page from
    pre-validation

 OvmfPkg/AmdSev/AmdSevX64.fdf          | 18 +++++++++---------
 OvmfPkg/ResetVector/ResetVector.nasmb | 15 ++++++++++++++-
 2 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf

[Dov Murik]

Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
matches the same order used in OvmfPkgX64.fdf.

After this change, this is the difference in the MEMFD of the two
targets:

$ diff -u \
       <(sed -ne '/FD.MEMFD/,/FV.SECFV/p' OvmfPkg/OvmfPkgX64.fdf) \
       <(sed -ne '/FD.MEMFD/,/FV.SECFV/p' OvmfPkg/AmdSev/AmdSevX64.fdf)
--- /dev/fd/63  2022-03-28 18:07:59.657531210 +0000
+++ /dev/fd/62  2022-03-28 18:07:59.657531210 +0000
@@ -32,6 +32,12 @@
 0x00E000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize

+0x00F000|0x000C00
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+0x00FC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.fdf | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 31f2be66361f..208f969cefc9 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -59,21 +59,21 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmf
 0x00B000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
 
-0x00C000|0x000C00
-gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
-
-0x00CC00|0x000400
-gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
-
-0x00D000|0x001000
+0x00C000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
 
-0x00E000|0x001000
+0x00D000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
 
-0x00F000|0x001000
+0x00E000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
 
+0x00F000|0x000C00
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+0x00FC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
-- 
2.20.1

Re: [PATCH 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf

[Gerd Hoffmann]

On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:
> Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
> matches the same order used in OvmfPkgX64.fdf.

Makes sense.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Maybe also move this to a common include file, so it is less likely that
they run out of sync again?

take care,
  Gerd

Re: [PATCH 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf

[Dov Murik]

Thanks Gerd for reviewing.

On 29/03/2022 14:36, Gerd Hoffmann wrote:
> On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:
>> Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
>> matches the same order used in OvmfPkgX64.fdf.
> 
> Makes sense.
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Thanks.

> 
> Maybe also move this to a common include file, so it is less likely that
> they run out of sync again?
> 

I was contemplating that, but wasn't sure (I only use OvmfPkgX64 and
AmdSevX64 in my testing).

Is it common in edk2?
Would it apply to other OvmfPkg targets? I see similar MEMFD in
CloudHvX64.fdf .

-Dov

Re: [PATCH 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf

[Gerd Hoffmann]

On Tue, Mar 29, 2022 at 03:32:36PM +0300, Dov Murik wrote:
> Thanks Gerd for reviewing.
> 
> On 29/03/2022 14:36, Gerd Hoffmann wrote:
> > On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:
> >> Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
> >> matches the same order used in OvmfPkgX64.fdf.
> > 
> > Makes sense.
> > 
> > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Thanks.
> 
> > 
> > Maybe also move this to a common include file, so it is less likely that
> > they run out of sync again?
> > 
> 
> I was contemplating that, but wasn't sure (I only use OvmfPkgX64 and
> AmdSevX64 in my testing).
> 
> Is it common in edk2?

We already have a few:

kraxel@sirius ~/projects/edk2 (gcc12)# ls OvmfPkg/*.fdf.inc 
OvmfPkg/FvmainCompactScratchEnd.fdf.inc  OvmfPkg/OvmfTpmDxe.fdf.inc  OvmfPkg/VarStore.fdf.inc
OvmfPkg/OvmfPkgDefines.fdf.inc           OvmfPkg/OvmfTpmPei.fdf.inc  OvmfPkg/XenElfHeader.fdf.inc

> Would it apply to other OvmfPkg targets? I see similar MEMFD in
> CloudHvX64.fdf .

I'd create one for the confidential computing memory areas,
that would only affect the builds which support SEV (and soon TDX).

Not sure about CloudHvX64.fdf, as far I know it does not support
SEV/TDX, maybe those antries are only there because they have been
copied over from OvmfPkgX64.fdf

take care,
  Gerd

Re: [PATCH 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf

[Dov Murik]

On 30/03/2022 8:14, Gerd Hoffmann wrote:
> On Tue, Mar 29, 2022 at 03:32:36PM +0300, Dov Murik wrote:
>> Thanks Gerd for reviewing.
>>
>> On 29/03/2022 14:36, Gerd Hoffmann wrote:
>>> On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:
>>>> Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
>>>> matches the same order used in OvmfPkgX64.fdf.
>>>
>>> Makes sense.
>>>
>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>
>> Thanks.
>>
>>>
>>> Maybe also move this to a common include file, so it is less likely that
>>> they run out of sync again?
>>>
>>
>> I was contemplating that, but wasn't sure (I only use OvmfPkgX64 and
>> AmdSevX64 in my testing).
>>
>> Is it common in edk2?
> 
> We already have a few:
> 
> kraxel@sirius ~/projects/edk2 (gcc12)# ls OvmfPkg/*.fdf.inc 
> OvmfPkg/FvmainCompactScratchEnd.fdf.inc  OvmfPkg/OvmfTpmDxe.fdf.inc  OvmfPkg/VarStore.fdf.inc
> OvmfPkg/OvmfPkgDefines.fdf.inc           OvmfPkg/OvmfTpmPei.fdf.inc  OvmfPkg/XenElfHeader.fdf.inc
> 

I saw these, but saw no !include directives in MEMFD areas, which are
more sensitive because the addresses and sizes must match the
surrounding definitions (unlike a list of INF directive like in
NetworkPkg/Network.fdf.inc or general settings like in
OvmfPkg/OvmfPkgDefines.fdf.inc).


>> Would it apply to other OvmfPkg targets? I see similar MEMFD in
>> CloudHvX64.fdf .
> 
> I'd create one for the confidential computing memory areas,
> that would only affect the builds which support SEV (and soon TDX).
> 

Almost all the MEMFD entries are somehow related to confidential
computing, isn't that the case?  For example PcdOvmfWorkAreaBase -- I
see it appears in the *.fdf of almost all targets.

I want to reduce duplication (= extract common parts to an .inc file),
but wonder what would be a clear and safe way to do it.

Suggestions:


Option 1:

Extract all the MEMFD entries starting from:

0x000000|0x006000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize

up to (including):

0x00E000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize


into OvmfMemFdPart1.fdf.inc, and !include it in OvmfPkgX64 and AmdSevX64.



Option 2:

Extract entire MEMFD part from OvmfPkgX64.fdf into OvmfMemFd.fdf.inc.

In the middle of it add something like:

!if $(SEV_LAUNCH_SECRET_ENABLE) == TRUE
0x00F000|0x000C00
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize

0x00FC00|0x000400
gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
!endif


and set that DEFINE in AmdSevX64.fdf only.




> Not sure about CloudHvX64.fdf, as far I know it does not support
> SEV/TDX, maybe those antries are only there because they have been
> copied over from OvmfPkgX64.fdf

The TDX series ("[PATCH V12 00/47] Enable Intel TDX in OvmfPkg
(Config-A)") modifies CloudHvX64.*, and also the CloudHv/README mentions
TDX.  So I assume they intend to support it.


-Dov

[PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

In order to allow the VMM (such as QEMU) to add a page with hashes of
kernel/initrd/cmdline for measured direct boot on SNP, this page must
not be part of the SNP metadata list reported to the VMM.

Check if that page is defined; if it is, skip it in the metadata list.
In such case, VMM should fill the page with the hashes content, or
explicitly update it as a zero page (if kernel hashes are not used).

Note that for SNP, the launch secret part of the page (lower 3KB) are
not relevant and will stay zero.  The last 1KB is used for the hashes.

This should have no effect on OvmfPkgX64 targets (which don't define
PcdSevLaunchSecretBase).

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/ResetVector/ResetVector.nasmb | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 9421f4818907..ac4c7e763b82 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -121,7 +121,20 @@
   ;
   %define SNP_SEC_MEM_BASE_DESC_2 (GHCB_BASE + 0x1000)
   %define SNP_SEC_MEM_SIZE_DESC_2 (SEV_SNP_SECRETS_BASE - SNP_SEC_MEM_BASE_DESC_2)
-  %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
+
+  %if (FixedPcdGet32 (PcdSevLaunchSecretBase) > 0)
+    ; There's a reserved page for SEV secrets and hashes; the VMM will fill and
+    ; validate the page, or mark it as a zero page.
+    %define EXPECTED_END_OF_LAUNCH_SECRET_PAGE (FixedPcdGet32 (PcdSevLaunchSecretBase) + \
+                                                FixedPcdGet32 (PcdSevLaunchSecretSize) + \
+                                                FixedPcdGet32 (PcdQemuHashTableSize))
+    %if (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) != EXPECTED_END_OF_LAUNCH_SECRET_PAGE)
+      %error "PcdOvmfSecPeiTempRamBase must start directly after the SEV Launch Secret page"
+    %endif
+    %define SNP_SEC_MEM_BASE_DESC_3 (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase))
+  %else
+    %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
+  %endif
   %define SNP_SEC_MEM_SIZE_DESC_3 (FixedPcdGet32 (PcdOvmfPeiMemFvBase) - SNP_SEC_MEM_BASE_DESC_3)
 
 %include "X64/IntelTdxMetadata.asm"
-- 
2.20.1

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Gerd Hoffmann]

  Hi,

> Check if that page is defined; if it is, skip it in the metadata list.
> In such case, VMM should fill the page with the hashes content, or
> explicitly update it as a zero page (if kernel hashes are not used).

Is it an option to just skip the page unconditionally?

I think in the OvmfPkgX64 build the page is not used, so it probably
doesn't matter whenever it is included or not, and it would make things
a bit less confusing ...

take care,
  Gerd

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

On 30/03/2022 8:20, Gerd Hoffmann wrote:
>   Hi,
> 
>> Check if that page is defined; if it is, skip it in the metadata list.
>> In such case, VMM should fill the page with the hashes content, or
>> explicitly update it as a zero page (if kernel hashes are not used).
> 
> Is it an option to just skip the page unconditionally?
> 
> I think in the OvmfPkgX64 build the page is not used, so it probably
> doesn't matter whenever it is included or not, and it would make things
> a bit less confusing ...
> 


Brijesh,

What would happen if we change this:

    %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)

to:

    %define SNP_SEC_MEM_BASE_DESC_3 (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase))

in OvmfPkg/ResetVector/ResetVector.nasmb ?

It means that the page starting at MEMFD_BASE_ADDRESS+0x00F000 (that is, the page
that follows the SNP CPUID page) will not be pre-validated by QEMU.

I'm not sure what are the implications.


-Dov

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Brijesh Singh]

On 3/30/22 01:04, Dov Murik wrote:
> 
> 
> On 30/03/2022 8:20, Gerd Hoffmann wrote:
>>    Hi,
>>
>>> Check if that page is defined; if it is, skip it in the metadata list.
>>> In such case, VMM should fill the page with the hashes content, or
>>> explicitly update it as a zero page (if kernel hashes are not used).
>>
>> Is it an option to just skip the page unconditionally?
>>
>> I think in the OvmfPkgX64 build the page is not used, so it probably
>> doesn't matter whenever it is included or not, and it would make things
>> a bit less confusing ...
>>
> 
> 
> Brijesh,
> 
> What would happen if we change this:
> 
>      %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
> 
> to:
> 
>      %define SNP_SEC_MEM_BASE_DESC_3 (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase))
> 
> in OvmfPkg/ResetVector/ResetVector.nasmb ?
> 
> It means that the page starting at MEMFD_BASE_ADDRESS+0x00F000 (that is, the page
> that follows the SNP CPUID page) will not be pre-validated by QEMU.
> 

Lets look at the OvmfPkgX64.fdf is

...

0x00E000|0x001000 
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize

0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

0x020000|0x0E0000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize

...

If you change SNP_SEC_MEM_BASE_DESC_3 to start from PcdOvmfPeiMemFvBase 
then who will validate the range for  PcdOvmfSecPeiTempRamBase - 
PcdOvmfPeiMemFvBase ? The SEC phase (Sec/X64/SecEntry.nasm) uses the 
PcdOvmfSecPeiTempRamBase. If the memory is not validated prior to use 
then it will result in #VC (page-not-validated) and crash the guest BIOS 
boot.

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

On 30/03/2022 22:27, Brijesh Singh wrote:
> 
> 
> On 3/30/22 01:04, Dov Murik wrote:
>>
>>
>> On 30/03/2022 8:20, Gerd Hoffmann wrote:
>>>    Hi,
>>>
>>>> Check if that page is defined; if it is, skip it in the metadata list.
>>>> In such case, VMM should fill the page with the hashes content, or
>>>> explicitly update it as a zero page (if kernel hashes are not used).
>>>
>>> Is it an option to just skip the page unconditionally?
>>>
>>> I think in the OvmfPkgX64 build the page is not used, so it probably
>>> doesn't matter whenever it is included or not, and it would make things
>>> a bit less confusing ...
>>>
>>
>>
>> Brijesh,
>>
>> What would happen if we change this:
>>
>>      %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
>>
>> to:
>>
>>      %define SNP_SEC_MEM_BASE_DESC_3 (FixedPcdGet32
>> (PcdOvmfSecPeiTempRamBase))
>>
>> in OvmfPkg/ResetVector/ResetVector.nasmb ?
>>
>> It means that the page starting at MEMFD_BASE_ADDRESS+0x00F000 (that
>> is, the page
>> that follows the SNP CPUID page) will not be pre-validated by QEMU.
>>
> 
> Lets look at the OvmfPkgX64.fdf is
> 
> ...
> 
> 0x00E000|0x001000
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
> 
> 
> 0x010000|0x010000
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> 
> 
> 0x020000|0x0E0000
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
> 
> 
> ...
> 
> If you change SNP_SEC_MEM_BASE_DESC_3 to start from PcdOvmfPeiMemFvBase
> then who will validate the range for  PcdOvmfSecPeiTempRamBase -
> PcdOvmfPeiMemFvBase ? The SEC phase (Sec/X64/SecEntry.nasm) uses the
> PcdOvmfSecPeiTempRamBase. If the memory is not validated prior to use
> then it will result in #VC (page-not-validated) and crash the guest BIOS
> boot.
> 

Gerd actually wants to change SNP_SEC_MEM_BASE_DESC_3 to start from
PcdOvmfSecPeiTempRamBase, which is 0x010000.

Supposedly no one uses the single page at 0x00F000 .

-Dov

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Brijesh Singh]

On 3/30/22 14:31, Dov Murik wrote:
> 
> 
> On 30/03/2022 22:27, Brijesh Singh wrote:
>>
>>
>> On 3/30/22 01:04, Dov Murik wrote:
>>>
>>>
>>> On 30/03/2022 8:20, Gerd Hoffmann wrote:
>>>>     Hi,
>>>>
>>>>> Check if that page is defined; if it is, skip it in the metadata list.
>>>>> In such case, VMM should fill the page with the hashes content, or
>>>>> explicitly update it as a zero page (if kernel hashes are not used).
>>>>
>>>> Is it an option to just skip the page unconditionally?
>>>>
>>>> I think in the OvmfPkgX64 build the page is not used, so it probably
>>>> doesn't matter whenever it is included or not, and it would make things
>>>> a bit less confusing ...
>>>>
>>>
>>>
>>> Brijesh,
>>>
>>> What would happen if we change this:
>>>
>>>       %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
>>>
>>> to:
>>>
>>>       %define SNP_SEC_MEM_BASE_DESC_3 (FixedPcdGet32
>>> (PcdOvmfSecPeiTempRamBase))
>>>
>>> in OvmfPkg/ResetVector/ResetVector.nasmb ?
>>>
>>> It means that the page starting at MEMFD_BASE_ADDRESS+0x00F000 (that
>>> is, the page
>>> that follows the SNP CPUID page) will not be pre-validated by QEMU.
>>>
>>
>> Lets look at the OvmfPkgX64.fdf is
>>
>> ...
>>
>> 0x00E000|0x001000
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
>>
>>
>> 0x010000|0x010000
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>
>>
>> 0x020000|0x0E0000
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>>
>>
>> ...
>>
>> If you change SNP_SEC_MEM_BASE_DESC_3 to start from PcdOvmfPeiMemFvBase
>> then who will validate the range for  PcdOvmfSecPeiTempRamBase -
>> PcdOvmfPeiMemFvBase ? The SEC phase (Sec/X64/SecEntry.nasm) uses the
>> PcdOvmfSecPeiTempRamBase. If the memory is not validated prior to use
>> then it will result in #VC (page-not-validated) and crash the guest BIOS
>> boot.
>>
> 
> Gerd actually wants to change SNP_SEC_MEM_BASE_DESC_3 to start from
> PcdOvmfSecPeiTempRamBase, which is 0x010000.
> 
> Supposedly no one uses the single page at 0x00F000 .

Yes, that should be alright as long as the SNP_SEC_MEM_BASE_DESC_3 start 
from PcdOvmfSecPeiTempRamBase. In PEI phase, we validate all the 
unvalidated range. So, as long as SEC phase is not using 800F000 - 
8010000 we should be good. The PEI will validate that page.

thanks

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

On 30/03/2022 22:35, Brijesh Singh wrote:
> 
> 
> On 3/30/22 14:31, Dov Murik wrote:
>>
>>
>> On 30/03/2022 22:27, Brijesh Singh wrote:
>>>
>>>
>>> On 3/30/22 01:04, Dov Murik wrote:
>>>>
>>>>
>>>> On 30/03/2022 8:20, Gerd Hoffmann wrote:
>>>>>     Hi,
>>>>>
>>>>>> Check if that page is defined; if it is, skip it in the metadata
>>>>>> list.
>>>>>> In such case, VMM should fill the page with the hashes content, or
>>>>>> explicitly update it as a zero page (if kernel hashes are not used).
>>>>>
>>>>> Is it an option to just skip the page unconditionally?
>>>>>
>>>>> I think in the OvmfPkgX64 build the page is not used, so it probably
>>>>> doesn't matter whenever it is included or not, and it would make
>>>>> things
>>>>> a bit less confusing ...
>>>>>
>>>>
>>>>
>>>> Brijesh,
>>>>
>>>> What would happen if we change this:
>>>>
>>>>       %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
>>>>
>>>> to:
>>>>
>>>>       %define SNP_SEC_MEM_BASE_DESC_3 (FixedPcdGet32
>>>> (PcdOvmfSecPeiTempRamBase))
>>>>
>>>> in OvmfPkg/ResetVector/ResetVector.nasmb ?
>>>>
>>>> It means that the page starting at MEMFD_BASE_ADDRESS+0x00F000 (that
>>>> is, the page
>>>> that follows the SNP CPUID page) will not be pre-validated by QEMU.
>>>>
>>>
>>> Lets look at the OvmfPkgX64.fdf is
>>>
>>> ...
>>>
>>> 0x00E000|0x001000
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
>>>
>>>
>>>
>>> 0x010000|0x010000
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>
>>>
>>>
>>> 0x020000|0x0E0000
>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>>>
>>>
>>>
>>> ...
>>>
>>> If you change SNP_SEC_MEM_BASE_DESC_3 to start from PcdOvmfPeiMemFvBase
>>> then who will validate the range for  PcdOvmfSecPeiTempRamBase -
>>> PcdOvmfPeiMemFvBase ? The SEC phase (Sec/X64/SecEntry.nasm) uses the
>>> PcdOvmfSecPeiTempRamBase. If the memory is not validated prior to use
>>> then it will result in #VC (page-not-validated) and crash the guest BIOS
>>> boot.
>>>
>>
>> Gerd actually wants to change SNP_SEC_MEM_BASE_DESC_3 to start from
>> PcdOvmfSecPeiTempRamBase, which is 0x010000.
>>
>> Supposedly no one uses the single page at 0x00F000 .
> 
> Yes, that should be alright as long as the SNP_SEC_MEM_BASE_DESC_3 start
> from PcdOvmfSecPeiTempRamBase. In PEI phase, we validate all the
> unvalidated range. So, as long as SEC phase is not using 800F000 -
> 8010000 we should be good. The PEI will validate that page.
> 


How does the PEI phase know that this page in the middle is still unvalidated?

I see this code:


STATIC SNP_PRE_VALIDATED_RANGE  mPreValidatedRange[] = {                    
  // The below address range was part of the SEV OVMF metadata, and range   
  // should be pre-validated by the Hypervisor.                             
  {                                                                         
    FixedPcdGet32 (PcdOvmfSecPageTablesBase),                               
    FixedPcdGet32 (PcdOvmfPeiMemFvBase),                                    
  },                                                                        
  // The below range is pre-validated by the Sec/SecMain.c                  
  {                                                                         
    FixedPcdGet32 (PcdOvmfSecValidatedStart),                               
    FixedPcdGet32 (PcdOvmfSecValidatedEnd)                                  
  },                                                                        
};                                                                          



As the comment says, it assumes the entire range
from PcdOvmfSecPageTablesBase (= 0x800000)
to PcdOvmfPeiMemFvBase (= 0x820000) 
is pre-validated by the Hypervisor.

How will it know to actually validate that page at 0x80F000 ?

-Dov

Re: [PATCH 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Gerd Hoffmann]

  Hi,

> >>>>>> Check if that page is defined; if it is, skip it in the metadata
> >>>>>> list.
> >>>>>> In such case, VMM should fill the page with the hashes content, or
> >>>>>> explicitly update it as a zero page (if kernel hashes are not used).
> >>>>>
> >>>>> Is it an option to just skip the page unconditionally?
> >>>>>
> >>>>> I think in the OvmfPkgX64 build the page is not used, so it probably
> >>>>> doesn't matter whenever it is included or not, and it would make
> >>>>> things
> >>>>> a bit less confusing ...

>   // The below address range was part of the SEV OVMF metadata, and range   
>   // should be pre-validated by the Hypervisor.                             
>   {                                                                         
>     FixedPcdGet32 (PcdOvmfSecPageTablesBase),                               
>     FixedPcdGet32 (PcdOvmfPeiMemFvBase),                                    
>   },                                                                        

> As the comment says, it assumes the entire range
> from PcdOvmfSecPageTablesBase (= 0x800000)
> to PcdOvmfPeiMemFvBase (= 0x820000) 
> is pre-validated by the Hypervisor.
> 
> How will it know to actually validate that page at 0x80F000 ?

Probably it doesn't unless we split the entry into two, so we are
effectively trading making the reset vector more complicated vs.
making this list more complicated.

I guess it's not worth the trouble then.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>
for or the original patch (and thanks for investigating).

take care,
  Gerd