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

["Laszlo Ersek" <lersek@redhat.com>]

On 06/08/21 17:43, Brijesh Singh wrote:
> 
> 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 ?

Well I'm sure I'm day-dreaming, but here's what I'd like, all at the
same time of course:

- to-the-point PCD names and explanations in the DEC file,

- minimize the new pages carved out of MEMFD,

- make sure that this new SEV-SNP secret page is reusable (at least
conceptually) in the "AmdSevX64.dsc" platform if necessary -- it may
exist at a different base address in that platform, but the mechanism
should be the same,

- I wouldn't like a new DXE driver for installing this new config table,
and I also wouldn't like to move and/or reuse SecretDxe. Instead, I
might like some new code in the existent AmdSevDxe driver, which is
already the SEV-specific platform driver (also included by the
"AmdSevX64.dsc" platform). If the config table cannot be installed in
the entry point function of the driver, can we invent a suitable
protocol notify, or other event notification callback?

> 
> 
>>> 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

Huh, so that's been intentional? I didn't expect it.

> is because we have seen that our docs
> folks change the link or they replace the old document with the new
> copy.

Changing links is tolerable, as long as previously released versions
(identified by document# and revision#) remain accessible forever.

> 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.

This is a fantastic idea from those kernel developers, and I'm scolding
myself for not thinking of the same.

If you do upload such documents to the kernel bugzilla, please feel free
to refer to the exact same (kernel bugziila) URLs in the edk2 commit
messages, and (perhaps) code comments. There's no need to upload the
(large) PDFs to the tianocore bugzilla separately; I trust the kernel
bugzilla instance to stick around.

> 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.

Hm, so the old docs do disappear for good (not just the URLs but the
docs themselves). In that case, please let us go with kernel bugzilla
attachments.

Thanks!
Laszlo