Re: [edk2-devel] [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD

["Brijesh Singh" <brijesh.singh@amd.com>]

On 6/8/21 4:20 AM, Laszlo Ersek via groups.io wrote:
>
> I thought the secrets page was entirely opaque to the guest firmware;
> i.e., all the guest firmware would do with it is (a) cover it with an
> allocation in SecretPei, (b) forward it to the guest OS via a UEFI
> system config table in SecretDxe.

Yes, it should be an opaque to the guest firmware. If someone wants to
do attestation inside the OVMF then they may need to know the whether
its an SEV or SEV-SNP guest. If its SEV guest then blob contains the
guest private information (such as disk key) and if its SEV-SNP guest
then blob contains the encryption key used for communicating with the PSP.


>
> This patch uses the same PCD names ("launch secret", where I understand
> the SEV-SNP case *not* to be a *launch* secret; is that right?), plus it
> uses the same drivers. That's way too confusing.

Yes, its not a launch secret. With your responses so far it seems that I
need to create a different PCD and spell out that its for SNP secrets
page, maybe something like this:

"PcdSevSnpSecretBase"

Since the code for reserving the memory type and installing the EFI
configuration table already existed in AmdSev/Secrets{Dxe,Pei} so I
decided to reuse. Now I understand that why you don't want to overload
those drivers. Those drivers are not part of OvmfPkgX64.dsc because the
generic packages do not support the attestation. I will update in v4 to
not use those drivers and reserve the memory during PEI phase and create
another driver to install the configuration table for it.

>
> So what is this "SNP secrets" page supposed to contain:
>
> - both the previously defined SEV/SEV-ES level launch secret, and the
> SNP-specific VMPCK (?)
>
> - how are these secret bits separated from each other in the page?
>
> - does the guest (firmware and/or OS) *write* to the new locations in
> the page, possibly for secure message construction?
>
>
> Either way, I think the proposed repurposing of the page, for the sake
> of SNP secrets (VMPCK and maybe even secure message construction?),
> breaks the current declarations of the PCDs, in "OvmfPkg.dec":
>
>   ## The base address and size of the SEV Launch Secret Area provisioned
>   #  after remote attestation.  If this is set in the .fdf, the platform
>   #  is responsible for protecting the area from DXE phase overwrites.
>   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
>   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43
>
>> In SEV-SNP, the secrets page is not tight up with just the remote
>> attestation.
> This is the most important statement. We need the SNP secrets page even
> without remote attestation. OvmfPkgX64.dsc does not deal with remote
> attestation.
>
> But then (putting all the PCD naming confusion aside), if a driver is
> promoted to "common use", from the AmdSevX64 platform to multiple
> OvmfPkg platforms, then it should be lifted to the top-level OvmfPkg
> directory.

Now I think about it maybe we should leave the driver where it is
because OvmfPkgX64.dsc does not need to deal with the attestation etc.
But we need to create a driver that can install the EFI configuration
table for the SNP secrets page. Is that okay ?


>> Later, the AmdSev.dsc can include a library to perform the
>> SEV-SNP-specific attestation. The library can use the SNP secrets page
>> to get the key and message counter use for constructing the guest
>> message to query the attestation report.
>>
>> I hope it clarifies it.
>>
>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F56860.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C972887a952cd48b090a308d92a5eb0d5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587409068253595%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=j%2FL5eTSR3%2FODJRE0IaF7R49aDVkFjDCO1JUpGsPbDwk%3D&reserved=0
>>
>>
>>> Honestly I'm getting a *rushed* vibe on this whole series. Why is that?
>> I am not sure why you are getting this feel, please let me know where I
>> can help to clarify but the series is *rushed* at all. Its building on
>> existing support. It's possible that we are getting mixed with the
>> fundamental difference between the SEV and SEV-SNP attestation flow and
>> recent patches from Dov to expand the attestation to cover other aspects
>> of the boot flow.
>>
>> In case of SEV-SNP, some folks may prefer to do all the attestation in
>> the OVMF and others may prefer to do the attestation in the guest OS. We
>> should try to not restrict one approach over the other.
>>
>>
>>> Assume that I'm dumb. You won't be far from the truth. Then hold my hand
>>> through all this?
>>
>> Please let me know if the above explanation helps or I should expand more.
> You should please (a) expand your *commit messages*, (b) add a *wall* of
> text in the "OvmfPkg.dec" file, where the PCDs in questions are
> declared. When I grep the OvmfPkg subdirectory in two years for
> "PcdSevLaunchSecretBase", I'd like to find the DEC file's comments to be
> consistent with the actual uses of the PCD, and I'd like git-blame to
> tell me something useful about those lines, too.
>
I will add more comments in the patch to clarify certain things.


> One problem is that I'm supposed to internalize about 50 pages from yet
> from another technical specification, in order to get the basics of a
> single patch. I can't even follow the *set* of AMD documents I should
> have a local copy of. How am I supposed to interleave all that with, for
> example, reviewing a 57 slide TDX design presentation?

As you may have seen that myself and Tom try not to put the exact link
or  document number in the comment is because we have seen that our docs
folks change the link or they replace the old document with the new
copy. We have similar issue in kernel. The kernel maintainer now have a
bugzilla where they want us to upload the document so that they can keep
a copy and in the commits we refer to that BZ link instead of AMD URL. I
myself gets so mixed up with various version of documents. I don't like
that we replace the old docs with a new without archiving it.

thanks