[PATCH v6 06/29] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase


Gerd Hoffmann
 

On Tue, Sep 14, 2021 at 03:49:31AM +0000, Yao, Jiewen wrote:
I can explain why we prefer DQ instead of DD.

You are right that current TD entrypoint is 32bit. However, we cannot predict that is always TRUE for the future.
So a "save space in MEMFD" vs. "be future proof" tradeoff.

Back to 16 bit machine, we have entrypoint at F000:FFF0. But in 32bit mode we removed 1M limitation and move entrypoint to below 4G.
There is no limitation that we move 64bit entrypoint above 4G for 64bit machine.
Any plans to actually do that?

Defining an 64-bit entry point is a more than just using 64-bit
addresses. For starters long mode requires paging being enabled.
So a set of initial page tables is needed, and you possibly need
to handle virtual vs. physical addresses. You also need to find
a place above 4G where you can place the firmware without conflicts
with something else.

So I have my doubts that trying to make the struct future-proof
will actually work out in practice. Or that we will actually
need that anytime soon.

take care,
Gerd


Yao, Jiewen
 

I can explain why we prefer DQ instead of DD.

You are right that current TD entrypoint is 32bit. However, we cannot predict that is always TRUE for the future.

Back to 16 bit machine, we have entrypoint at F000:FFF0. But in 32bit mode we removed 1M limitation and move entrypoint to below 4G.
There is no limitation that we move 64bit entrypoint above 4G for 64bit machine.
We still choose 4G for easy implementation and compatibility consideration.

And we still leave room for extension in the future. For example, the firmware information table (FIT) defines 64bit address, although only 32bit is used today. (https://software.intel.com/content/dam/develop/external/us/en/documents/firmware-interface-table-bios-specification-r1p2p1.pdf).

Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
Hoffmann
Sent: Tuesday, September 7, 2021 3:08 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Brijesh Singh <brijesh.singh@amd.com>; James
Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Justen, Jordan L
<jordan.l.justen@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Erdem Aktas <erdemaktas@google.com>; Michael Roth
<Michael.Roth@amd.com>
Subject: Re: [edk2-devel] [PATCH v6 06/29] OvmfPkg/ResetVector: pre-validate
the data pages used in SEC phase

Hi,

[ Looking at https://www.mail-
archive.com/devel@edk2.groups.io/msg33605.html ]

So, there isn't much tdx-specific in tdx-metadata. Most ranges are
TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means
these ranges should be accepted by the hypervisor, which is pretty much the
same issue snp tries to solve with this pre-validation range. Then there are
the ranges for code (aka bfv), for vars (aka cfv) and td_hob.

td_hob is the only tdx-specific item there, and even that concept (pass
memory ranges as hob list from hypervisor to guest) might be useful outside
tdx.
Mailbox is tdx-specific too. But Stack/Heap/OvmfWorkarea/OvmfPageTable
are
common. BFV/CFV are common too.
Mailbox is tagged "TDX_METADATA_SECTION_TYPE_TEMP_MEM", so nothing
special to do when loading the firmware, right?

I'd suggest we generalize the tdx-metadata idea and define both generic and
vmm-specific section types:

enum {
OVMF_SECTION_TYPE_UNDEFINED = 0;

/* generic */
OVMF_SECTION_TYPE_CODE = 0x100,
OVMF_SECTION_TYPE_VARS
OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */

/* sev */
OVMF_SECTION_TYPE_SEV_SECRETS = 0x200,
OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */

/* tdx */
OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300,
};

Comments?
TDX has similar section type.
Yes. Both TDX and SNP have simliar requirements, they want store memory
ranges in the firmware binary in a way that allows qemu finding them and
using them when initializing the guest.

SNP stores the ranges directly in the GUID-chained block in the reset
vector. The range types are implicit (first is pre-validate area,
second is cpuid page, ...).

TDX stores a pointer to tdx-metadata in the GUID-chained block, then the
tdx-metadata has a list of ranges. The ranges are explicitly typed
(section type field).

The indirection used by TDX keeps the reset vector small. Also the
explicit typing of the ranges makes it easier to extend later on if
needed.

IMHO SEV should at minimum add explicit types to the memory ranges in
the boot block, but I'd very much prefer it if SEV and TDX can agree
on a way to store the memory ranges.

But I am not sure if SEV can use this metadata mechanism.
Need SEV's comments.
Brijesh?

Looking at tdx-metadata I have a few questions:

+_Bfv:
+ DD TDX_BFV_RAW_DATA_OFFSET
+ DD TDX_BFV_RAW_DATA_SIZE

What is this and why is it needed?
Host VMM need to measure the code part (BFV) to MRTD register
(which is similar to TPM PCRs).
Sure, but why you can't use TDX_BFV_MEMORY_BASE +
TDX_BFV_MEMORY_SIZE
for that? The tdvf design guide says it is the file offset. The
firmware must be mapped right below 4G, which implies
RAW_DATA_OFFSET + (4G - FIRMWARE_SIZE) == MEMORY_BASE.
So having both RAW_DATA_OFFSET and MEMORY_BASE looks redundant to me.

+ DQ TDX_BFV_MEMORY_BASE
+ DQ TDX_BFV_MEMORY_SIZE

Why "DQ"? TDX is defined to start in 32bit mode, so you can hardly have
addresses here which do not fit into "DD", correct?
Those are the memory. TDX is running in long mode. So it is DQ.
Hmm? TDX entry vector is defined to be 32bit. Which pretty much
implies you can't map the firmware above 4G, even if one of the first
actions of the reset vector code is the switch to long mode.

take care,
Gerd





Min Xu
 

Hi, Brijesh
When I go thru the code I find a potential bug in MemEncryptSevEsIsEnabled().

In the current code both SEV and TDX leverage the OvmfWorkArea to record the SEV/TDX information.
Byte [0] record the guest type, 0 for legacy guest. 1 for sev, 2 for tdx.
Byte [3:1] are reserved.
From Byte[4] on its meaning depends on the guest type (byte[0]).
For SEV it is SEC_SEV_ES_WORK_AREA.

In the InternalMemEncryptSevStatus(), it check the SEC_SEV_ES_WORK_AREA directly without checking if it is SEV guest. (byte[0])
SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);

I am afraid it is not correct. Would you please double check it?

Thanks!
Min

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Tuesday, September 7, 2021 9:27 PM
To: kraxel@redhat.com; Xu, Min M <min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; James Bottomley
<jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
<erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>
Subject: Re: [edk2-devel] [PATCH v6 06/29] OvmfPkg/ResetVector: pre-validate
the data pages used in SEC phase



On 9/7/21 2:07 AM, kraxel@redhat.com wrote:
Hi,

[ Looking at
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
w.mail-
%2F&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb2dfcc7e0f93
4cacdce408d971ce2f5a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%
7C63
7665952633333113%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
CJQIjo
iV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WsSaU8kFv
w1
NWES%2BYOw7xENZr9cpPgQvBjsXWkc8nkg%3D&amp;reserved=0
archive.com/devel@edk2.groups.io/msg33605.html ]

So, there isn't much tdx-specific in tdx-metadata. Most ranges are
TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means
these ranges should be accepted by the hypervisor, which is pretty
much the same issue snp tries to solve with this pre-validation
range. Then there are the ranges for code (aka bfv), for vars (aka cfv) and
td_hob.

td_hob is the only tdx-specific item there, and even that concept
(pass memory ranges as hob list from hypervisor to guest) might be
useful outside tdx.
Mailbox is tdx-specific too. But
Stack/Heap/OvmfWorkarea/OvmfPageTable are common. BFV/CFV are
common too.

Mailbox is tagged "TDX_METADATA_SECTION_TYPE_TEMP_MEM", so nothing
special to do when loading the firmware, right?

I'd suggest we generalize the tdx-metadata idea and define both
generic and vmm-specific section types:

enum {
OVMF_SECTION_TYPE_UNDEFINED = 0;

/* generic */
OVMF_SECTION_TYPE_CODE = 0x100,
OVMF_SECTION_TYPE_VARS
OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */

/* sev */
OVMF_SECTION_TYPE_SEV_SECRETS = 0x200,
OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */

/* tdx */
OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300, };

Comments?
TDX has similar section type.
Yes. Both TDX and SNP have simliar requirements, they want store
memory ranges in the firmware binary in a way that allows qemu finding
them and using them when initializing the guest.

SNP stores the ranges directly in the GUID-chained block in the reset
vector. The range types are implicit (first is pre-validate area,
second is cpuid page, ...).

TDX stores a pointer to tdx-metadata in the GUID-chained block, then
the tdx-metadata has a list of ranges. The ranges are explicitly
typed (section type field).

The indirection used by TDX keeps the reset vector small. Also the
explicit typing of the ranges makes it easier to extend later on if
needed.

IMHO SEV should at minimum add explicit types to the memory ranges in
the boot block, but I'd very much prefer it if SEV and TDX can agree
on a way to store the memory ranges.

But I am not sure if SEV can use this metadata mechanism.
Need SEV's comments.
Brijesh?
We should be able to make use of the metadata approach for the SEV-SNP.
I will update the SNP patches to use the metadata approach in next rev.

thanks


Brijesh Singh
 

On 9/7/21 2:07 AM, kraxel@redhat.com wrote:
Hi,

[ Looking at https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-%2F&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb2dfcc7e0f934cacdce408d971ce2f5a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665952633333113%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WsSaU8kFvw1NWES%2BYOw7xENZr9cpPgQvBjsXWkc8nkg%3D&amp;reserved=0
archive.com/devel@edk2.groups.io/msg33605.html ]

So, there isn't much tdx-specific in tdx-metadata. Most ranges are
TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means
these ranges should be accepted by the hypervisor, which is pretty much the
same issue snp tries to solve with this pre-validation range. Then there are
the ranges for code (aka bfv), for vars (aka cfv) and td_hob.

td_hob is the only tdx-specific item there, and even that concept (pass
memory ranges as hob list from hypervisor to guest) might be useful outside
tdx.
Mailbox is tdx-specific too. But Stack/Heap/OvmfWorkarea/OvmfPageTable are
common. BFV/CFV are common too.
Mailbox is tagged "TDX_METADATA_SECTION_TYPE_TEMP_MEM", so nothing
special to do when loading the firmware, right?

I'd suggest we generalize the tdx-metadata idea and define both generic and
vmm-specific section types:

enum {
OVMF_SECTION_TYPE_UNDEFINED = 0;

/* generic */
OVMF_SECTION_TYPE_CODE = 0x100,
OVMF_SECTION_TYPE_VARS
OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */

/* sev */
OVMF_SECTION_TYPE_SEV_SECRETS = 0x200,
OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */

/* tdx */
OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300,
};

Comments?
TDX has similar section type.
Yes. Both TDX and SNP have simliar requirements, they want store memory
ranges in the firmware binary in a way that allows qemu finding them and
using them when initializing the guest.
SNP stores the ranges directly in the GUID-chained block in the reset
vector. The range types are implicit (first is pre-validate area,
second is cpuid page, ...).
TDX stores a pointer to tdx-metadata in the GUID-chained block, then the
tdx-metadata has a list of ranges. The ranges are explicitly typed
(section type field).
The indirection used by TDX keeps the reset vector small. Also the
explicit typing of the ranges makes it easier to extend later on if
needed.
IMHO SEV should at minimum add explicit types to the memory ranges in
the boot block, but I'd very much prefer it if SEV and TDX can agree
on a way to store the memory ranges.

But I am not sure if SEV can use this metadata mechanism.
Need SEV's comments.
Brijesh?
We should be able to make use of the metadata approach for the SEV-SNP. I will update the SNP patches to use the metadata approach in next rev.

thanks


Gerd Hoffmann
 

Hi,

[ Looking at https://www.mail-
archive.com/devel@edk2.groups.io/msg33605.html ]

So, there isn't much tdx-specific in tdx-metadata. Most ranges are
TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means
these ranges should be accepted by the hypervisor, which is pretty much the
same issue snp tries to solve with this pre-validation range. Then there are
the ranges for code (aka bfv), for vars (aka cfv) and td_hob.

td_hob is the only tdx-specific item there, and even that concept (pass
memory ranges as hob list from hypervisor to guest) might be useful outside
tdx.
Mailbox is tdx-specific too. But Stack/Heap/OvmfWorkarea/OvmfPageTable are
common. BFV/CFV are common too.
Mailbox is tagged "TDX_METADATA_SECTION_TYPE_TEMP_MEM", so nothing
special to do when loading the firmware, right?

I'd suggest we generalize the tdx-metadata idea and define both generic and
vmm-specific section types:

enum {
OVMF_SECTION_TYPE_UNDEFINED = 0;

/* generic */
OVMF_SECTION_TYPE_CODE = 0x100,
OVMF_SECTION_TYPE_VARS
OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */

/* sev */
OVMF_SECTION_TYPE_SEV_SECRETS = 0x200,
OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */

/* tdx */
OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300,
};

Comments?
TDX has similar section type.
Yes. Both TDX and SNP have simliar requirements, they want store memory
ranges in the firmware binary in a way that allows qemu finding them and
using them when initializing the guest.

SNP stores the ranges directly in the GUID-chained block in the reset
vector. The range types are implicit (first is pre-validate area,
second is cpuid page, ...).

TDX stores a pointer to tdx-metadata in the GUID-chained block, then the
tdx-metadata has a list of ranges. The ranges are explicitly typed
(section type field).

The indirection used by TDX keeps the reset vector small. Also the
explicit typing of the ranges makes it easier to extend later on if
needed.

IMHO SEV should at minimum add explicit types to the memory ranges in
the boot block, but I'd very much prefer it if SEV and TDX can agree
on a way to store the memory ranges.

But I am not sure if SEV can use this metadata mechanism.
Need SEV's comments.
Brijesh?

Looking at tdx-metadata I have a few questions:

+_Bfv:
+ DD TDX_BFV_RAW_DATA_OFFSET
+ DD TDX_BFV_RAW_DATA_SIZE

What is this and why is it needed?
Host VMM need to measure the code part (BFV) to MRTD register
(which is similar to TPM PCRs).
Sure, but why you can't use TDX_BFV_MEMORY_BASE + TDX_BFV_MEMORY_SIZE
for that? The tdvf design guide says it is the file offset. The
firmware must be mapped right below 4G, which implies
RAW_DATA_OFFSET + (4G - FIRMWARE_SIZE) == MEMORY_BASE.
So having both RAW_DATA_OFFSET and MEMORY_BASE looks redundant to me.

+ DQ TDX_BFV_MEMORY_BASE
+ DQ TDX_BFV_MEMORY_SIZE

Why "DQ"? TDX is defined to start in 32bit mode, so you can hardly have
addresses here which do not fit into "DD", correct?
Those are the memory. TDX is running in long mode. So it is DQ.
Hmm? TDX entry vector is defined to be 32bit. Which pretty much
implies you can't map the firmware above 4G, even if one of the first
actions of the reset vector code is the switch to long mode.

take care,
Gerd


Min Xu
 

On September 6, 2021 8:17 PM, Gerd Hoffman wrote:
Hi,

sevSnpBootBlockStart:
+ DD SNP_HV_VALIDATED_START
+ DD SNP_HV_VALIDATED_END
We pack all the Tdx information into a blob (TdxMetadata). These tdx
information Includes the BFV(i.e. OVMF_CODE.fd), the CFV(i.e.
OVMF_VARS.fd), TdMailbox, etc.
The offset to the TdxMetadata is in the GUIDed chain in
ResetVectorVtf0.asm.

In the future new metadata can be added into the TdxMetadata without
changes in ResetVectorVtf0.asm.
[ Looking at https://www.mail-
archive.com/devel@edk2.groups.io/msg33605.html ]

So, there isn't much tdx-specific in tdx-metadata. Most ranges are
TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means
these ranges should be accepted by the hypervisor, which is pretty much the
same issue snp tries to solve with this pre-validation range. Then there are
the ranges for code (aka bfv), for vars (aka cfv) and td_hob.

td_hob is the only tdx-specific item there, and even that concept (pass
memory ranges as hob list from hypervisor to guest) might be useful outside
tdx.
Mailbox is tdx-specific too. But Stack/Heap/OvmfWorkarea/OvmfPageTable are
common. BFV/CFV are common too.

So, can we settle on one approach for this please? I think the tdx-metadata
style approach is more flexible and future-proof. It can easily be extended
without changing data structures, we only need new section types. I expect
this will work better long-term when it comes to backward-compatibility.

I'd suggest we generalize the tdx-metadata idea and define both generic and
vmm-specific section types:

enum {
OVMF_SECTION_TYPE_UNDEFINED = 0;

/* generic */
OVMF_SECTION_TYPE_CODE = 0x100,
OVMF_SECTION_TYPE_VARS
OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */

/* sev */
OVMF_SECTION_TYPE_SEV_SECRETS = 0x200,
OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */

/* tdx */
OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300,
};

Comments?
TDX has similar section type.
But I am not sure if SEV can use this metadata mechanism.
Need SEV's comments.

Looking at tdx-metadata I have a few questions:

+_Bfv:
+ DD TDX_BFV_RAW_DATA_OFFSET
+ DD TDX_BFV_RAW_DATA_SIZE

What is this and why is it needed?
Host VMM need to measure the code part (BFV) to MRTD register
(which is similar to TPM PCRs).

+ DQ TDX_BFV_MEMORY_BASE
+ DQ TDX_BFV_MEMORY_SIZE

Why "DQ"? TDX is defined to start in 32bit mode, so you can hardly have
addresses here which do not fit into "DD", correct?
Those are the memory. TDX is running in long mode. So it is DQ.

+ DD TDX_METADATA_SECTION_TYPE_BFV
+ DD TDX_METADATA_ATTRIBUTES_EXTENDMR

What does this attribute mean?
It indicates if the host VMM should use TDCALL[TDH.MR.EXTEND] for this section.
https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf
Please refer to Section 11 TDVF Metadata for more detailed information.
Note: this Section will be updated according to the comments from the community.
Thanks!
Min


Gerd Hoffmann
 

Hi,

sevSnpBootBlockStart:
+ DD SNP_HV_VALIDATED_START
+ DD SNP_HV_VALIDATED_END
We pack all the Tdx information into a blob (TdxMetadata). These tdx information
Includes the BFV(i.e. OVMF_CODE.fd), the CFV(i.e. OVMF_VARS.fd), TdMailbox, etc.
The offset to the TdxMetadata is in the GUIDed chain in ResetVectorVtf0.asm.
In the future new metadata can be added into the TdxMetadata without changes
in ResetVectorVtf0.asm.
[ Looking at https://www.mail-archive.com/devel@edk2.groups.io/msg33605.html ]

So, there isn't much tdx-specific in tdx-metadata. Most ranges are
TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means these
ranges should be accepted by the hypervisor, which is pretty much the
same issue snp tries to solve with this pre-validation range. Then
there are the ranges for code (aka bfv), for vars (aka cfv) and td_hob.

td_hob is the only tdx-specific item there, and even that concept (pass
memory ranges as hob list from hypervisor to guest) might be useful
outside tdx.

So, can we settle on one approach for this please? I think the
tdx-metadata style approach is more flexible and future-proof. It can
easily be extended without changing data structures, we only need new
section types. I expect this will work better long-term when it comes
to backward-compatibility.

I'd suggest we generalize the tdx-metadata idea and define both generic
and vmm-specific section types:

enum {
OVMF_SECTION_TYPE_UNDEFINED = 0;

/* generic */
OVMF_SECTION_TYPE_CODE = 0x100,
OVMF_SECTION_TYPE_VARS
OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */

/* sev */
OVMF_SECTION_TYPE_SEV_SECRETS = 0x200,
OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */

/* tdx */
OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300,
};

Comments?

Looking at tdx-metadata I have a few questions:

+_Bfv:
+ DD TDX_BFV_RAW_DATA_OFFSET
+ DD TDX_BFV_RAW_DATA_SIZE

What is this and why is it needed?

+ DQ TDX_BFV_MEMORY_BASE
+ DQ TDX_BFV_MEMORY_SIZE

Why "DQ"? TDX is defined to start in 32bit mode, so you can hardly have
addresses here which do not fit into "DD", correct?

+ DD TDX_METADATA_SECTION_TYPE_BFV
+ DD TDX_METADATA_ATTRIBUTES_EXTENDMR

What does this attribute mean?

take care,
Gerd


Min Xu
 

On September 2, 2021 4:20 PM, Gerd Hoffmann wrote:
Hi,

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.
Hmm, tdx must handle this too.

+
+
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedStart|0x0
+ |UINT32|0x56
+
+
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedEnd|0x0|
U
+ INT32|0x57
So maybe we should drop the "Snp" from the name here ...

; GUID (SEV-SNP boot block): bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9
;
sevSnpBootBlockStart:
+ DD SNP_HV_VALIDATED_START
+ DD SNP_HV_VALIDATED_END
... and store the range which needs validation in another, not snp-specific
block?

Jiewen? Min?
We pack all the Tdx information into a blob (TdxMetadata). These tdx information
Includes the BFV(i.e. OVMF_CODE.fd), the CFV(i.e. OVMF_VARS.fd), TdMailbox, etc.
The offset to the TdxMetadata is in the GUIDed chain in ResetVectorVtf0.asm.
;
; GUID : e47a6535-984a-4798-865e-4685a7bf8ec2
;
tdxMetadataOffsetStart:
DD (OVMF_IMAGE_SIZE_IN_KB * 1024 - (fourGigabytes - TdxMetadataGuid - 16))
DW tdxMetadataOffsetEnd - tdxMetadataOffsetStart
DB 0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
DB 0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
tdxMetadataOffsetEnd:

In the future new metadata can be added into the TdxMetadata without changes
in ResetVectorVtf0.asm.

Thanks!
Min


Gerd Hoffmann
 

Hi,

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.
Hmm, tdx must handle this too.

+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedStart|0x0|UINT32|0x56
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedEnd|0x0|UINT32|0x57
So maybe we should drop the "Snp" from the name here ...

; GUID (SEV-SNP boot block): bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9
;
sevSnpBootBlockStart:
+ DD SNP_HV_VALIDATED_START
+ DD SNP_HV_VALIDATED_END
... and store the range which needs validation in another, not snp-specific block?

Jiewen? Min?

take care,
Gerd


Brijesh Singh
 

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.

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>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/OvmfPkg.dec | 5 +++++
OvmfPkg/OvmfPkgX64.fdf | 6 ++++--
OvmfPkg/ResetVector/ResetVector.inf | 2 ++
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 5 +++++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 ++
5 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index afe9b7135560..41fffd6d3bd9 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -353,6 +353,11 @@ [PcdsFixedAtBuild]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|0|UINT32|0x54
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize|0|UINT32|0x55

+ ## The start and end of pre-validated memory region by the hypervisor
+ # through the SEV-SNP firmware.
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedStart|0x0|UINT32|0x56
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedEnd|0x0|UINT32|0x57
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 1e292d11ace3..10e8ba758a29 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -110,9 +110,11 @@ [FD.MEMFD]
#
SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
-##########################################################################################

-################################################################################
+# The range of the pages pre-validated through the SEV-SNP firmware.
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedStart = $(MEMFD_BASE_ADDRESS) + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedEnd = $(MEMFD_BASE_ADDRESS) + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
+##########################################################################################

[FV.SECFV]
FvNameGuid = 763BED0D-DE9F-48F5-81F1-3E90E1B1A015
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index 34d843de62c4..307699d8bf22 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -54,3 +54,5 @@ [FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedStart
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpHypervisorPreValidatedEnd
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 71e1484cf4e4..a45e828a04a5 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -57,9 +57,14 @@ guidedStructureStart:
; SEV-SNP boot block GUID and provide the GPA to the PSP to populate
; the memory area with the required information..
;
+; In order to boot the SEV-SNP guest the hypervisor must pre-validated the
+; memory range from SNP_HV_VALIDATED_START to SNP_HV_VALIDATED_END.
+;
; GUID (SEV-SNP boot block): bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9
;
sevSnpBootBlockStart:
+ DD SNP_HV_VALIDATED_START
+ DD SNP_HV_VALIDATED_END
DD SNP_SECRETS_BASE
DD SNP_SECRETS_SIZE
DD SNP_CPUID_BASE
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 9be963206989..75a38697d5d8 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -81,6 +81,8 @@
%define SNP_CPUID_SIZE FixedPcdGet32 (PcdOvmfSnpCpuidSize)
%define SNP_SECRETS_BASE FixedPcdGet32 (PcdOvmfSnpSecretsBase)
%define SNP_SECRETS_SIZE FixedPcdGet32 (PcdOvmfSnpSecretsSize)
+ %define SNP_HV_VALIDATED_START FixedPcdGet32 (PcdOvmfSnpHypervisorPreValidatedStart)
+ %define SNP_HV_VALIDATED_END FixedPcdGet32 (PcdOvmfSnpHypervisorPreValidatedEnd)
%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/AmdSev.asm"
%include "Ia32/PageTables64.asm"
--
2.17.1