1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] OvmfPkg/AmdSev: Erase secret area content on ExitBootServices

Dov Murik posted 1 patch 2 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20211102082506.366921-1-dovmurik@linux.ibm.com
OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf |  2 +
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 47 ++++++++++++++++++--
2 files changed, 45 insertions(+), 4 deletions(-)
[edk2-devel] [PATCH] OvmfPkg/AmdSev: Erase secret area content on ExitBootServices
Posted by Dov Murik 2 years, 5 months ago 
The confidential computing secrets area is marked as EfiBootServicesData
region, which means it is released for the OS use when the OS EFI stub
calls ExitBootServices.  However, its content is not erased, and
therefore the OS might unintentionally reuse this sensitive memory area
and expose the injected secrets.

Erase the content of the secret area on ExitBootServices so that the
memory released to the OS contains zeros.  If the OS needs to keep the
secrets for its own use, it must copy the secrets area to another memory
area before calling ExitBootServices (for example in efi/libstub in
Linux).

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---

Code is in: https://github.com/confidential-containers-demo/edk2/tree/erase-secret-area

---
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf |  2 +
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 47 ++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
index 40bda7ff846c..ff831afaeb66 100644
--- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
@@ -23,6 +23,8 @@ [Packages]
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
+  BaseMemoryLib
+  DebugLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
 
diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
index 934ad207632b..085759f0e523 100644
--- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
@@ -5,6 +5,8 @@
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 #include <PiDxe.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Guid/ConfidentialComputingSecret.h>
 
@@ -13,6 +15,35 @@ STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
   FixedPcdGet32 (PcdSevLaunchSecretSize),
 };
 
+STATIC EFI_EVENT mSecretDxeExitBootEvent;
+
+/**
+  ExitBootServices event notification function for the secret table.
+
+  This function erases the content of the secret area so the secrets don't leak
+  via released BootServices memory.  If the OS wants to keep the secrets for
+  its own use, it must copy the secrets area to another memory area before
+  calling ExitBootServices (for example in efi/libstub in Linux).
+
+  @param[in] Event         The ExitBoot event that has been signaled.
+
+  @param[in] Context       Unused.
+**/
+STATIC
+VOID
+EFIAPI
+SecretDxeExitBoot (
+  IN EFI_EVENT Event,
+  IN VOID      *Context
+  )
+{
+  ASSERT(mSecretDxeTable.Base != 0);
+  ASSERT(mSecretDxeTable.Size > 0);
+
+  ZeroMem ((VOID *) ((UINTN) mSecretDxeTable.Base), mSecretDxeTable.Size);
+}
+
+
 EFI_STATUS
 EFIAPI
 InitializeSecretDxe(
@@ -20,8 +51,16 @@ InitializeSecretDxe(
   IN EFI_SYSTEM_TABLE     *SystemTable
   )
 {
-  return gBS->InstallConfigurationTable (
-                &gConfidentialComputingSecretGuid,
-                &mSecretDxeTable
-                );
+  EFI_STATUS Status;
+
+  Status = gBS->InstallConfigurationTable (
+                  &gConfidentialComputingSecretGuid,
+                  &mSecretDxeTable
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  return gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
+                SecretDxeExitBoot, NULL, &mSecretDxeExitBootEvent);
 }
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83100): https://edk2.groups.io/g/devel/message/83100
Mute This Topic: https://groups.io/mt/86761563/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/AmdSev: Erase secret area content on ExitBootServices
Posted by Gerd Hoffmann 2 years, 5 months ago 
On Tue, Nov 02, 2021 at 08:25:06AM +0000, Dov Murik wrote:
> The confidential computing secrets area is marked as EfiBootServicesData
> region, which means it is released for the OS use when the OS EFI stub
> calls ExitBootServices.  However, its content is not erased, and
> therefore the OS might unintentionally reuse this sensitive memory area
> and expose the injected secrets.
> 
> Erase the content of the secret area on ExitBootServices so that the
> memory released to the OS contains zeros.  If the OS needs to keep the
> secrets for its own use, it must copy the secrets area to another memory
> area before calling ExitBootServices (for example in efi/libstub in
> Linux).

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83125): https://edk2.groups.io/g/devel/message/83125
Mute This Topic: https://groups.io/mt/86761563/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] OvmfPkg/AmdSev: Erase secret area content on ExitBootServices
Posted by Dov Murik 2 years, 5 months ago 
Please don't merge this.  We're going in a different direction, see
https://edk2.groups.io/g/devel/message/83853 .  Instead of letting the
guest kernel copy the secret content and OVMF will erase the original
(the patch below), we mark the area as "reserved" (in OVMF) and then the
OS doesn't need to copy it around.  This is also similar to the approach
taken in the SNP patches for the SNP-Secrets and SNP-CPUID pages.

Added bonus is that it's less code both in OVMF and in kernel's efi and
efi/libstub.

Thanks,
-Dov



On 02/11/2021 10:25, Dov Murik wrote:
> The confidential computing secrets area is marked as EfiBootServicesData
> region, which means it is released for the OS use when the OS EFI stub
> calls ExitBootServices.  However, its content is not erased, and
> therefore the OS might unintentionally reuse this sensitive memory area
> and expose the injected secrets.
> 
> Erase the content of the secret area on ExitBootServices so that the
> memory released to the OS contains zeros.  If the OS needs to keep the
> secrets for its own use, it must copy the secrets area to another memory
> area before calling ExitBootServices (for example in efi/libstub in
> Linux).
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
> 
> Code is in: https://github.com/confidential-containers-demo/edk2/tree/erase-secret-area
> 
> ---
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf |  2 +
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 47 ++++++++++++++++++--
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> index 40bda7ff846c..ff831afaeb66 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> @@ -23,6 +23,8 @@ [Packages]
>    MdePkg/MdePkg.dec
> 
>  
> 
>  [LibraryClasses]
> 
> +  BaseMemoryLib
> 
> +  DebugLib
> 
>    UefiBootServicesTableLib
> 
>    UefiDriverEntryPoint
> 
>  
> 
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> index 934ad207632b..085759f0e523 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -5,6 +5,8 @@
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
>  #include <PiDxe.h>
> 
> +#include <Library/BaseMemoryLib.h>
> 
> +#include <Library/DebugLib.h>
> 
>  #include <Library/UefiBootServicesTableLib.h>
> 
>  #include <Guid/ConfidentialComputingSecret.h>
> 
>  
> 
> @@ -13,6 +15,35 @@ STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
>    FixedPcdGet32 (PcdSevLaunchSecretSize),
> 
>  };
> 
>  
> 
> +STATIC EFI_EVENT mSecretDxeExitBootEvent;
> 
> +
> 
> +/**
> 
> +  ExitBootServices event notification function for the secret table.
> 
> +
> 
> +  This function erases the content of the secret area so the secrets don't leak
> 
> +  via released BootServices memory.  If the OS wants to keep the secrets for
> 
> +  its own use, it must copy the secrets area to another memory area before
> 
> +  calling ExitBootServices (for example in efi/libstub in Linux).
> 
> +
> 
> +  @param[in] Event         The ExitBoot event that has been signaled.
> 
> +
> 
> +  @param[in] Context       Unused.
> 
> +**/
> 
> +STATIC
> 
> +VOID
> 
> +EFIAPI
> 
> +SecretDxeExitBoot (
> 
> +  IN EFI_EVENT Event,
> 
> +  IN VOID      *Context
> 
> +  )
> 
> +{
> 
> +  ASSERT(mSecretDxeTable.Base != 0);
> 
> +  ASSERT(mSecretDxeTable.Size > 0);
> 
> +
> 
> +  ZeroMem ((VOID *) ((UINTN) mSecretDxeTable.Base), mSecretDxeTable.Size);
> 
> +}
> 
> +
> 
> +
> 
>  EFI_STATUS
> 
>  EFIAPI
> 
>  InitializeSecretDxe(
> 
> @@ -20,8 +51,16 @@ InitializeSecretDxe(
>    IN EFI_SYSTEM_TABLE     *SystemTable
> 
>    )
> 
>  {
> 
> -  return gBS->InstallConfigurationTable (
> 
> -                &gConfidentialComputingSecretGuid,
> 
> -                &mSecretDxeTable
> 
> -                );
> 
> +  EFI_STATUS Status;
> 
> +
> 
> +  Status = gBS->InstallConfigurationTable (
> 
> +                  &gConfidentialComputingSecretGuid,
> 
> +                  &mSecretDxeTable
> 
> +                  );
> 
> +  if (EFI_ERROR (Status)) {
> 
> +    return Status;
> 
> +  }
> 
> +
> 
> +  return gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> 
> +                SecretDxeExitBoot, NULL, &mSecretDxeExitBootEvent);
> 
>  }
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83854): https://edk2.groups.io/g/devel/message/83854
Mute This Topic: https://groups.io/mt/86761563/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.