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@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Tobin Feldman-Fitzthum <tobin@...>
Signed-off-by: Dov Murik <dovmurik@...>
---
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

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

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

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.

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

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

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

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@...>
for or the original patch (and thanks for investigating).

take care,
Gerd