[edk2-devel] [PATCH 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as runtime memory

[Dov Murik]

Thanks Ard for reviewing this patch.

On 09/12/2022 1:02, Ard Biesheuvel wrote:
> On Thu, 8 Dec 2022 at 09:08, Dov Murik <dovmurik at linux.ibm.com> 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 it 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 AllocateRuntimePool
>> to ensure the guest OS will not reuse this memory.
>>
> 
> This memory type is mapped into the EFI page tables, and omitted from
> the linear map in Linux on arm64, so it is generally not the right
> type for data that only has significance to the OS. In spite of the
> name, EfiAcpiReclaimMemory is more suitable here - the OS is free to
> preserve it or treat it as ordinary memory.

Just making sure -- this data might be useful in grub (if we embed grub
into OVMF to boot encrypted disk from an SEV injected launch secret)
and/or in Linux (module efi_secret will try to access this same area).

Both need access to this small table and to the secret page itself.


> 
> I realise that this is not of great importance here given that the
> table is only 8 bytes in size, but if we can, I'd prefer it if we use
> ACPI reclaim memory here.
> 

I assume I need to use gBS->AllocatePool() in order to specify this
special memory type.

I'll try it and see if it solves the issue I'm experiencing.


Thanks,
-Dov

>> Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved")
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>> Cc: James Bottomley <jejb at linux.ibm.com>
>> Cc: Jiewen Yao <Jiewen.Yao at intel.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Tobin Feldman-Fitzthum <tobin at linux.ibm.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Signed-off-by: Dov Murik <dovmurik at linux.ibm.com>
>> ---
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf |  2 ++
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 17 +++++++++++------
>>  2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>> index 40bda7ff846c..67d35f19b063 100644
>> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>> @@ -23,6 +23,8 @@ [Packages]
>>    MdePkg/MdePkg.dec
>>
>>  [LibraryClasses]
>> +  DebugLib
>> +  MemoryAllocationLib
>>    UefiBootServicesTableLib
>>    UefiDriverEntryPoint
>>
>> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>> index 3d84b2545052..615dff6cbf59 100644
>> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>> @@ -5,14 +5,11 @@
>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>>  **/
>>  #include <PiDxe.h>
>> +#include <Library/DebugLib.h>
>>  #include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/MemoryAllocationLib.h> // AllocateRuntimePool()
>>  #include <Guid/ConfidentialComputingSecret.h>
>>
>> -STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION  mSecretDxeTable = {
>> -  FixedPcdGet32 (PcdSevLaunchSecretBase),
>> -  FixedPcdGet32 (PcdSevLaunchSecretSize),
>> -};
>> -
>>  EFI_STATUS
>>  EFIAPI
>>  InitializeSecretDxe (
>> @@ -20,8 +17,16 @@ InitializeSecretDxe (
>>    IN EFI_SYSTEM_TABLE  *SystemTable
>>    )
>>  {
>> +  CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
>> +
>> +  SecretDxeTable = AllocateRuntimePool (sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION));
>> +  ASSERT (SecretDxeTable != NULL);
>> +
>> +  SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
>> +  SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
>> +
>>    return gBS->InstallConfigurationTable (
>>                  &gConfidentialComputingSecretGuid,
>> -                &mSecretDxeTable
>> +                SecretDxeTable
>>                  );
>>  }
>> --
>> 2.25.1
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97160): https://edk2.groups.io/g/devel/message/97160
Mute This Topic: https://groups.io/mt/95533916/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-