Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory

["Dov Murik" <dovmurik@linux.ibm.com>]

Thank you Jiewen.

On 15/12/2022 4:42, Yao, Jiewen wrote:
> Hey
> CI failed - https://github.com/tianocore/edk2/pull/3772
> Have you run CI before submit patch? Please take a look.

I haven't -- last time I contributed to edk2 was a long time ago; I'll
look for the instructions on triggering the CI myself.

I see patch format errors (lines a bit too long) and uncrustify errors
(missing space in variable declaration).  I'll fix these and send
another spin.

-Dov

> 
> Thank you
> Yao, Jiewen
> 
>> -----Original Message-----
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> Sent: Monday, December 12, 2022 11:01 PM
>> To: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io
>> Cc: Tobin Feldman-Fitzthum <tobin@ibm.com>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org>; Aktas, Erdem <erdemaktas@google.com>;
>> Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
>> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan
>> L <jordan.l.justen@intel.com>; Michael Roth <michael.roth@amd.com>; Xu,
>> Min M <min.m.xu@intel.com>; Tobin Feldman-Fitzthum
>> <tobin@linux.ibm.com>
>> Subject: Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret
>> location as EfiACPIReclaimMemory
>>
>> On 12/12/22 02:08, Dov Murik wrote:
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4186
>>>
>>> Commit 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch
>> secret
>>> area as reserved") marked the launch secret area itself (1 page) as
>>> reserved so the guest OS can use it during the lifetime of the OS.
>>> However, the address and size of the secret area held in the
>>> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct are declared as
>> STATIC in
>>> OVMF (in AmdSev/SecretDxe); therefore there's no guarantee that it will
>>> not be written over by OS data.
>>>
>>> Fix this by allocating the memory for the
>>> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct with the
>>> EfiACPIReclaimMemory memory type to ensure the guest OS will not reuse
>>> this memory.
>>>
>>> Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch
>> secret area as reserved")
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <Jiewen.Yao@intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Michael Roth <michael.roth@amd.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>>
>>>
>>> ---
>>>
>>> v2 changes:
>>> * Allocate with EfiACPIReclaimMemory memory type (thanks Ard)
>>> ---
>>>   OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 22 ++++++++++++++------
>>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>> b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>>> index 3d84b2545052..4f65b1ce5ba5 100644
>>> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>>> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>>> @@ -8,11 +8,6 @@
>>>   #include <Library/UefiBootServicesTableLib.h>
>>>   #include <Guid/ConfidentialComputingSecret.h>
>>>
>>> -STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION
>> mSecretDxeTable = {
>>> -  FixedPcdGet32 (PcdSevLaunchSecretBase),
>>> -  FixedPcdGet32 (PcdSevLaunchSecretSize),
>>> -};
>>> -
>>>   EFI_STATUS
>>>   EFIAPI
>>>   InitializeSecretDxe (
>>> @@ -20,8 +15,23 @@ InitializeSecretDxe (
>>>     IN EFI_SYSTEM_TABLE  *SystemTable
>>>     )
>>>   {
>>> +  EFI_STATUS                             Status;
>>> +  CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
>>> +
>>> +  Status = gBS->AllocatePool (
>>> +                  EfiACPIReclaimMemory,
>>> +                  sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION),
>>> +                  (VOID **)&SecretDxeTable
>>> +                  );
>>> +  if (EFI_ERROR (Status)) {
>>> +    return Status;
>>> +  }
>>> +
>>> +  SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
>>> +  SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
>>> +
>>>     return gBS->InstallConfigurationTable (
>>>                   &gConfidentialComputingSecretGuid,
>>> -                &mSecretDxeTable
>>> +                SecretDxeTable
>>>                   );
>>>   }