Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table

["Yao, Jiewen" <jiewen.yao@intel.com>]

I see your point. I will let the patch submitter make final decision on which way they want to go.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, December 10, 2020 5:12 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> jejb@linux.ibm.com
> Cc: dovmurik@linux.vnet.ibm.com; Dov.Murik1@il.ibm.com;
> ashish.kalra@amd.com; brijesh.singh@amd.com; tobin@ibm.com;
> david.kaplan@amd.com; jon.grimm@amd.com; thomas.lendacky@amd.com;
> frankeh@us.ibm.com; Dr . David Alan Gilbert <dgilbert@redhat.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>
> Subject: Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev
> Secret area using a configuration table
> 
> Hi Jiewen,
> 
> On 12/09/20 13:02, Yao, Jiewen wrote:
> > Hi James
> > I am not sure if this solution is only for AMD SEV or it is a generic solution
> to "pass a secret to grub and let grub decrypt the disk".
> >
> > If it is only for AMD SEV, please stop reading and ignore my comment
> below.
> >
> > If it is designed to be a generic solution to pass a secret to grub and let
> grub decrypt the disk. I have some thought below:
> > Intel TDX
> (https://software.intel.com/content/www/us/en/develop/articles/intel-
> trust-domain-extensions.html) have similar feature - a TDX virtual firmware
> may do attestation and get a key from remote key server.
> > We might use same architecture to pass the secrete to grub.
> > Initially, we define an ACPI 'SVKL' table to pass the secrete in intel-tdx-
> guest-hypervisor-communication-interface
> (https://software.intel.com/content/dam/develop/external/us/en/document
> s/intel-tdx-guest-hypervisor-communication-interface.pdf), section 4.4
> storage volume key data.
> > But it is also OK if you want to use UEFI configuration table.
> >
> > If we need a common API for both AMD SEV and Intel TDX, then I
> recommend some enhancement for SevLaunchSecret.h.
> > 1) The file name (SevLaunchSecret.h) should be generic, such as
> TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'.
> Otherwise, we have to define a new GUID for 'TDX'.
> > 2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID)
> should be generic. Same reason above.
> > 3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be
> generic. Same reason above.
> > 4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should
> use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory
> location.
> > 5) The internal data structure of the secret is not defined. Is it raw binary?
> Or ASCII string password? Or DER format certificate? Or PEM format key? At
> least, we shall describe it in the header file.
> > 6) The might be a chance that a key server need input multiple keys to a
> trusted VM. How we handle this? Do we expect multiple UEFI configuration
> tables and each table support one key? or one table to support multiple
> keys?
> >
> > Would you please take a look at intel-tdx-guest-hypervisor-
> communication-interface, section 4.4 storage volume key data.
> > We defined multiple key layout, key type and key format. Please let us
> know if you have any thought.
> 
> These are several change requests. I do not disagree with them, but I
> strongly propose we implement them separately.
> 
> James's current v3 series presents a state that has been tested and
> reviewed. I'd like to commit it as-is. If for nothing else, then because
> I'd like the edk2 git commit history to have a snapshot of the work at
> this stage.
> 
> We have ample time until the next edk2 stable tag. Right after this v3
> series is committed, you guys can start generalizing it (e.g., renaming
> files and variables). Working from special case to general case is not
> uncommon. The feature need not be ready as soon as it is committed; it
> needs to be stable and externally compatible at the next stable tag.
> 
> If some changes from your list above would be incompatible with other
> software (which is also in the making, to my understanding), then I
> would request / propose that patches for those other projects be held
> back, until the generalization of the edk2 patches reaches a certain
> maturity.
> 
> Basically, I wouldn't like to do an incremental review on this series
> that introduces the above-described *amount* of changes, from v3 to v4.
> And I would like to perform a from-the-scratch review even less, on this
> series. I believe your requests have merit, I'd just like to see patches
> "on top" that implement them. We're right after the last stable tag,
> this is the time for some incompatibility, until things settle.
> 
> Thanks
> Laszlo