[PATCH v3 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate secret location as EfiACPIReclaimMemory

[PATCH v3 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate secret location as EfiACPIReclaimMemory

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

---

v3 changes:
* Whitespace fix

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..c3258570e941 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
                 );
 }
-- 
2.25.1

Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate secret location as EfiACPIReclaimMemory

[Ard Biesheuvel]

On Thu, 15 Dec 2022 at 14:12, 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 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
> 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>
>
> ---
>
> v3 changes:
> * Whitespace fix
>

Merged as #3777

Thanks,


> 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..c3258570e941 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
>                  );
>  }
> --
> 2.25.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#97464): https://edk2.groups.io/g/devel/message/97464
> Mute This Topic: https://groups.io/mt/95687884/1131722
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> ------------
>
>