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

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

On 06/07/21 19:33, Brijesh Singh wrote:
> 
> On 6/7/21 7:48 AM, Laszlo Ersek wrote:
>> On 06/07/21 14:26, Laszlo Ersek wrote:
>>> On 05/27/21 01:11, Brijesh Singh wrote:
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489236720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=OjlLNpUW8U%2FykMMU7JwjddEZd9Zi%2BsHNK%2FqoQCwW3vo%3D&reserved=0
>>>>
>>>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
>>>> secrets page.
>>> For pure SEV?
>>>
>>>> When SEV-SNP is enabled, the secrets page contains the VM platform
>>>> communication keys. The guest BIOS and OS can use this key to communicate
>>>> with the SEV firmware to get attesation report. See the SEV-SNP firmware
>>>> spec for more details for the content of the secrets page.
>>>>
>>>> When SEV and SEV-ES is enabled, the secrets page contains the information
>>>> provided by the guest owner after the attestation. See the SEV
>>>> LAUNCH_SECRET command for more details.
>>>>
>>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>>> Cc: Min Xu <min.m.xu@intel.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>> ---
>>>>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>>>>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.c   | 15 ++++++++++++++-
>>>>  4 files changed, 22 insertions(+), 1 deletion(-)
>>> How is all of the above related to the "OvmfPkg/OvmfPkgX64.dsc"
>>> platform, where remote attestation is not a goal?
>>>
>>> What you describe makes sense to me, but only for the remote-attested
>>> "OvmfPkg/AmdSev/AmdSevX64.dsc" platform. (Which already includes
>>> SecretPei and SecretDxe, and sets the necessary PCDs.)
>>>
>>> Then, even if we limit this patch only to the "OvmfPkg/AmdSev/SecretPei"
>>> module, the commit message does not explain sufficiently why the secrets
>>> page must be reserved for good. The "SEV-SNP firmware spec" reference is
>>> vague at best; I'm permanently lost between the dozen PDF files I have
>>> downloaded locally from the AMD website. Please include a specific
>>> document number, revision number, and chapter/section identifier.
>>>
>>> Honestly I'm getting a *rushed* vibe on this whole series. Why is that?
>>>
>>> Assume that I'm dumb. You won't be far from the truth. Then hold my hand
>>> through all this?
>> Here's the v2 discussion:
>>
>> - https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F9804ecb5-8afd-c56e-4982-d1a6ebad3de8%40redhat.com&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489236720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=K8FRcks19dQ4BM4DBOh%2F7uO4hNvIsM0eqdNvwUQzDUU%3D&amp;reserved=0
>> - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F74797&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489236720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8rfpRAEvBdWex0BQctCbbGnHb691gcKSIEvVA3ZKDkg%3D&amp;reserved=0
>> - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00112.html&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NAL8jAfiq1EApkDBOBjgL7b3NIsmjginZSDxB1NDCk8%3D&amp;reserved=0
>>
>> That discussion refers to a different use case, raised by Dov. That use
>> case might justify reserving the area even for plain SEV. It's out of
>> scope for now, AIUI.
>>
>> (
>>
>> And even for that separate use case, James showed down-thread that *not*
>> reserving the page forever in the firmware is more flexible.
>>
>> - https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2Faed7d3490fe6edee74440ed8e4cd5364fb2ba4af.camel%40linux.ibm.com&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2UV6KcGYb9CoKzgIU%2FscCX2l%2F5pKaSkFYshP%2BPSWHSM%3D&amp;reserved=0
>> - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F74801&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=jnHpYxYkijt2LtcH772m88%2BLNH3Zjfn3Zqc3uuttL1M%3D&amp;reserved=0
>> - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00116.html&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=XzePjtTDS8blsXhBNOg52uo81uFhoYpgcNMvU4RupSI%3D&amp;reserved=0
>>
>> )
>>
>> AFAICT, the only effect of the v2 sub-thread on the patch has been that
>> we now use the Reserved memory type rather than AcpiNVS (when SEV-SNP is
>> in use). I have two comments on that:
>>
>> - It's good that we're not mixing in the other use case raised by Dov
>>   (i.e., enabling the guest-kernel to read secrets from the injected
>>   page even under plain SEV).
>>
>> - It's still unclear to me why the reservation needs to be permanent
>>   under SEV-SNP.
> 
> 
> As highlighted in the previous email, in the case of SEV, the secrets
> page contains the private data provided by the guest owner to the guest.
> Whereas, in SEV-SNP, the secrets page includes the key and other
> metadata used by the guest (OVMF or kernel) to construct a message for
> the PSP. The secrets page contains some information (e.g key and
> sequence number) that must persist across kexec boots. If we mark the
> SEV-SNP secrets page as "Boot Data," I believe it gets free'd on
> ExitBootService(). In the kexec'ed kernel, we need to retrieve the
> secret page to get the key and message counters to construct the next
> PSP quest request command.

All great information, especially the kexec bits; why not explicitly
document all this?

Thanks
Laszlo

> 
> 
> I have not looked into detail on how EFI configuration table and other
> data is preserved during the kexec boot, but I thought making the
> secrets reserved should ensure that memory is not free'd on
> ExitBootServices() and we can reach it after the kexec. I can
> investigate a bit more.
> 
> Dov/James,
>  
> Does kexec works with the SEV secrets page?
> 
> -Brijesh
> 
> 
> 
>