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

["Min Xu" <min.m.xu@intel.com>]

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