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

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

[Dov Murik]

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.

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

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

[Ard Biesheuvel]

On Thu, 8 Dec 2022 at 09:08, Dov Murik <dovmurik@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.

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.

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

Re: [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@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@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: 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>
>> ---
>>  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
>>