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 <dovmu...@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+tianoc...@kernel.org>
>> Cc: Erdem Aktas <erdemak...@google.com>
>> Cc: Gerd Hoffmann <kra...@redhat.com>
>> Cc: James Bottomley <j...@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen....@intel.com>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Min Xu <min.m...@intel.com>
>> Cc: Tobin Feldman-Fitzthum <to...@linux.ibm.com>
>> Cc: Tom Lendacky <thomas.lenda...@amd.com>
>> Signed-off-by: Dov Murik <dovmu...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
