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


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

Join devel@edk2.groups.io to automatically receive all group messages.