Now that the secret area is protected by a boot time HOB, extract its
location details into a configuration table referenced by
gSevLaunchSecretGuid so the boot loader or OS can locate it before a
call to ExitBootServices().

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkg.dec                    |  1 +
 OvmfPkg/AmdSev/AmdSevX64.dsc           |  1 +
 OvmfPkg/AmdSev/AmdSevX64.fdf           |  1 +
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf | 37 ++++++++++++++++++++++++++
 OvmfPkg/Include/Guid/SevLaunchSecret.h | 28 +++++++++++++++++++
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 26 ++++++++++++++++++
 6 files changed, 94 insertions(+)
 create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
 create mode 100644 OvmfPkg/Include/Guid/SevLaunchSecret.h
 create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7d27f8e16040..8a294116efaa 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -117,6 +117,7 @@ [Guids]
   gLinuxEfiInitrdMediaGuid              = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
   gQemuKernelLoaderFsMediaGuid          = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
   gGrubFileGuid                         = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
+  gSevLaunchSecretGuid                  = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
 
 [Ppis]
   # PPI whose presence in the PPI database signals that the TPM base address
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index e9c522bedad9..bb7697eb324b 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -778,6 +778,7 @@ [Components]
       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
   }
 !endif
+  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
   OvmfPkg/AmdSev/Grub/Grub.inf
 !if $(BUILD_SHELL) == TRUE
   ShellPkg/Application/Shell/Shell.inf {
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index b2656a1cf6fc..e8fd4b8c7b89 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -269,6 +269,7 @@ [FV.DXEFV]
 !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(BUILD_SHELL) == TRUE
 INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 !endif
+INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
 INF  OvmfPkg/AmdSev/Grub/Grub.inf
 !if $(BUILD_SHELL) == TRUE
 INF  ShellPkg/Application/Shell/Shell.inf
diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
new file mode 100644
index 000000000000..62ab00a3d382
--- /dev/null
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
@@ -0,0 +1,37 @@
+## @file
+#  Sev Secret configuration Table installer
+#
+#  Copyright (C) 2020 James Bottomley, IBM Corporation.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = SecretDxe
+  FILE_GUID                      = 6e2b9619-8810-4e9d-a177-d432bb9abeda
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = InitializeSecretDxe
+
+[Sources]
+  SecretDxe.c
+
+[Packages]
+  OvmfPkg/OvmfPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  UefiBootServicesTableLib
+  UefiDriverEntryPoint
+
+[Guids]
+  gSevLaunchSecretGuid
+
+[FixedPcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+[Depex]
+  TRUE
diff --git a/OvmfPkg/Include/Guid/SevLaunchSecret.h b/OvmfPkg/Include/Guid/SevLaunchSecret.h
new file mode 100644
index 000000000000..fa5f3830bc2b
--- /dev/null
+++ b/OvmfPkg/Include/Guid/SevLaunchSecret.h
@@ -0,0 +1,28 @@
+ /** @file
+   UEFI Configuration Table for exposing the SEV Launch Secret location to UEFI
+   applications (boot loaders).
+
+   Copyright (C) 2020 James Bottomley, IBM Corporation.
+   SPDX-License-Identifier: BSD-2-Clause-Patent
+ **/
+
+#ifndef SEV_LAUNCH_SECRET_H_
+#define SEV_LAUNCH_SECRET_H_
+
+#include <Uefi/UefiBaseType.h>
+
+#define SEV_LAUNCH_SECRET_GUID                          \
+  { 0xadf956ad,                                         \
+    0xe98c,                                             \
+    0x484c,                                             \
+    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47 }, \
+  }
+
+typedef struct {
+  UINT32 Base;
+  UINT32 Size;
+} SEV_LAUNCH_SECRET_LOCATION;
+
+extern EFI_GUID gSevLaunchSecretGuid;
+
+#endif // SEV_LAUNCH_SECRET_H_
diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
new file mode 100644
index 000000000000..d8cc9b00946a
--- /dev/null
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
@@ -0,0 +1,26 @@
+/** @file
+  SEV Secret configuration table constructor
+
+  Copyright (C) 2020 James Bottomley, IBM Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+#include <PiDxe.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Guid/SevLaunchSecret.h>
+
+STATIC SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = {
+  FixedPcdGet32 (PcdSevLaunchSecretBase),
+  FixedPcdGet32 (PcdSevLaunchSecretSize),
+};
+
+EFI_STATUS
+EFIAPI
+InitializeSecretDxe(
+  IN EFI_HANDLE           ImageHandle,
+  IN EFI_SYSTEM_TABLE     *SystemTable
+  )
+{
+  return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid,
+                                         &mSecretDxeTable
+                                         );
+}
-- 
2.26.2



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

Hi James
I am not sure if this solution is only for AMD SEV or it is a generic solution to "pass a secret to grub and let grub decrypt the disk".

If it is only for AMD SEV, please stop reading and ignore my comment below.

If it is designed to be a generic solution to pass a secret to grub and let grub decrypt the disk. I have some thought below:
Intel TDX (https://software.intel.com/content/www/us/en/develop/articles/intel-trust-domain-extensions.html) have similar feature - a TDX virtual firmware may do attestation and get a key from remote key server.
We might use same architecture to pass the secrete to grub.
Initially, we define an ACPI 'SVKL' table to pass the secrete in intel-tdx-guest-hypervisor-communication-interface (https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface.pdf), section 4.4 storage volume key data.
But it is also OK if you want to use UEFI configuration table.

If we need a common API for both AMD SEV and Intel TDX, then I recommend some enhancement for SevLaunchSecret.h.
1) The file name (SevLaunchSecret.h) should be generic, such as TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'. Otherwise, we have to define a new GUID for 'TDX'.
2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID) should be generic. Same reason above.
3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be generic. Same reason above.
4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory location.
5) The internal data structure of the secret is not defined. Is it raw binary? Or ASCII string password? Or DER format certificate? Or PEM format key? At least, we shall describe it in the header file.
6) The might be a chance that a key server need input multiple keys to a trusted VM. How we handle this? Do we expect multiple UEFI configuration tables and each table support one key? or one table to support multiple keys?

Would you please take a look at intel-tdx-guest-hypervisor-communication-interface, section 4.4 storage volume key data.
We defined multiple key layout, key type and key format. Please let us know if you have any thought.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of James
> Bottomley
> Sent: Tuesday, December 1, 2020 4:28 AM
> To: devel@edk2.groups.io
> Cc: dovmurik@linux.vnet.ibm.com; Dov.Murik1@il.ibm.com;
> ashish.kalra@amd.com; brijesh.singh@amd.com; tobin@ibm.com;
> david.kaplan@amd.com; jon.grimm@amd.com; thomas.lendacky@amd.com;
> jejb@linux.ibm.com; frankeh@us.ibm.com; Dr . David Alan Gilbert
> <dgilbert@redhat.com>; Laszlo Ersek <lersek@redhat.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev
> Secret area using a configuration table
> 
> Now that the secret area is protected by a boot time HOB, extract its
> location details into a configuration table referenced by
> gSevLaunchSecretGuid so the boot loader or OS can locate it before a
> call to ExitBootServices().
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/OvmfPkg.dec                    |  1 +
>  OvmfPkg/AmdSev/AmdSevX64.dsc           |  1 +
>  OvmfPkg/AmdSev/AmdSevX64.fdf           |  1 +
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf | 37
> ++++++++++++++++++++++++++
>  OvmfPkg/Include/Guid/SevLaunchSecret.h | 28 +++++++++++++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 26 ++++++++++++++++++
>  6 files changed, 94 insertions(+)
>  create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>  create mode 100644 OvmfPkg/Include/Guid/SevLaunchSecret.h
>  create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 7d27f8e16040..8a294116efaa 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -117,6 +117,7 @@ [Guids]
>    gLinuxEfiInitrdMediaGuid              = {0x5568e427, 0x68fc, 0x4f3d, {0xac,
> 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
>    gQemuKernelLoaderFsMediaGuid          = {0x1428f772, 0xb64a, 0x441e,
> {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
>    gGrubFileGuid                         = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62,
> 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
> +  gSevLaunchSecretGuid                  = {0xadf956ad, 0xe98c, 0x484c, {0xae,
> 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
> 
>  [Ppis]
>    # PPI whose presence in the PPI database signals that the TPM base
> address
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index e9c522bedad9..bb7697eb324b 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -778,6 +778,7 @@ [Components]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>    }
>  !endif
> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>    OvmfPkg/AmdSev/Grub/Grub.inf
>  !if $(BUILD_SHELL) == TRUE
>    ShellPkg/Application/Shell/Shell.inf {
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf
> b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index b2656a1cf6fc..e8fd4b8c7b89 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -269,6 +269,7 @@ [FV.DXEFV]
>  !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(BUILD_SHELL) == TRUE
>  INF
> OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellComma
> nd.inf
>  !endif
> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>  INF  OvmfPkg/AmdSev/Grub/Grub.inf
>  !if $(BUILD_SHELL) == TRUE
>  INF  ShellPkg/Application/Shell/Shell.inf
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> new file mode 100644
> index 000000000000..62ab00a3d382
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> @@ -0,0 +1,37 @@
> +## @file
> +#  Sev Secret configuration Table installer
> +#
> +#  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = SecretDxe
> +  FILE_GUID                      = 6e2b9619-8810-4e9d-a177-d432bb9abeda
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = InitializeSecretDxe
> +
> +[Sources]
> +  SecretDxe.c
> +
> +[Packages]
> +  OvmfPkg/OvmfPkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Guids]
> +  gSevLaunchSecretGuid
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
> +
> +[Depex]
> +  TRUE
> diff --git a/OvmfPkg/Include/Guid/SevLaunchSecret.h
> b/OvmfPkg/Include/Guid/SevLaunchSecret.h
> new file mode 100644
> index 000000000000..fa5f3830bc2b
> --- /dev/null
> +++ b/OvmfPkg/Include/Guid/SevLaunchSecret.h
> @@ -0,0 +1,28 @@
> + /** @file
> +   UEFI Configuration Table for exposing the SEV Launch Secret location to
> UEFI
> +   applications (boot loaders).
> +
> +   Copyright (C) 2020 James Bottomley, IBM Corporation.
> +   SPDX-License-Identifier: BSD-2-Clause-Patent
> + **/
> +
> +#ifndef SEV_LAUNCH_SECRET_H_
> +#define SEV_LAUNCH_SECRET_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +#define SEV_LAUNCH_SECRET_GUID                          \
> +  { 0xadf956ad,                                         \
> +    0xe98c,                                             \
> +    0x484c,                                             \
> +    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47 }, \
> +  }
> +
> +typedef struct {
> +  UINT32 Base;
> +  UINT32 Size;
> +} SEV_LAUNCH_SECRET_LOCATION;
> +
> +extern EFI_GUID gSevLaunchSecretGuid;
> +
> +#endif // SEV_LAUNCH_SECRET_H_
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> new file mode 100644
> index 000000000000..d8cc9b00946a
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -0,0 +1,26 @@
> +/** @file
> +  SEV Secret configuration table constructor
> +
> +  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +#include <PiDxe.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Guid/SevLaunchSecret.h>
> +
> +STATIC SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = {
> +  FixedPcdGet32 (PcdSevLaunchSecretBase),
> +  FixedPcdGet32 (PcdSevLaunchSecretSize),
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +InitializeSecretDxe(
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_SYSTEM_TABLE     *SystemTable
> +  )
> +{
> +  return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid,
> +                                         &mSecretDxeTable
> +                                         );
> +}
> --
> 2.26.2
> 
> 
> 
> 
> 



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

Hi Jiewen,

On 12/09/20 13:02, Yao, Jiewen wrote:
> Hi James
> I am not sure if this solution is only for AMD SEV or it is a generic solution to "pass a secret to grub and let grub decrypt the disk".
> 
> If it is only for AMD SEV, please stop reading and ignore my comment below.
> 
> If it is designed to be a generic solution to pass a secret to grub and let grub decrypt the disk. I have some thought below:
> Intel TDX (https://software.intel.com/content/www/us/en/develop/articles/intel-trust-domain-extensions.html) have similar feature - a TDX virtual firmware may do attestation and get a key from remote key server.
> We might use same architecture to pass the secrete to grub.
> Initially, we define an ACPI 'SVKL' table to pass the secrete in intel-tdx-guest-hypervisor-communication-interface (https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface.pdf), section 4.4 storage volume key data.
> But it is also OK if you want to use UEFI configuration table.
> 
> If we need a common API for both AMD SEV and Intel TDX, then I recommend some enhancement for SevLaunchSecret.h.
> 1) The file name (SevLaunchSecret.h) should be generic, such as TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'. Otherwise, we have to define a new GUID for 'TDX'.
> 2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID) should be generic. Same reason above.
> 3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be generic. Same reason above.
> 4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory location.
> 5) The internal data structure of the secret is not defined. Is it raw binary? Or ASCII string password? Or DER format certificate? Or PEM format key? At least, we shall describe it in the header file.
> 6) The might be a chance that a key server need input multiple keys to a trusted VM. How we handle this? Do we expect multiple UEFI configuration tables and each table support one key? or one table to support multiple keys?
> 
> Would you please take a look at intel-tdx-guest-hypervisor-communication-interface, section 4.4 storage volume key data.
> We defined multiple key layout, key type and key format. Please let us know if you have any thought.

These are several change requests. I do not disagree with them, but I
strongly propose we implement them separately.

James's current v3 series presents a state that has been tested and
reviewed. I'd like to commit it as-is. If for nothing else, then because
I'd like the edk2 git commit history to have a snapshot of the work at
this stage.

We have ample time until the next edk2 stable tag. Right after this v3
series is committed, you guys can start generalizing it (e.g., renaming
files and variables). Working from special case to general case is not
uncommon. The feature need not be ready as soon as it is committed; it
needs to be stable and externally compatible at the next stable tag.

If some changes from your list above would be incompatible with other
software (which is also in the making, to my understanding), then I
would request / propose that patches for those other projects be held
back, until the generalization of the edk2 patches reaches a certain
maturity.

Basically, I wouldn't like to do an incremental review on this series
that introduces the above-described *amount* of changes, from v3 to v4.
And I would like to perform a from-the-scratch review even less, on this
series. I believe your requests have merit, I'd just like to see patches
"on top" that implement them. We're right after the last stable tag,
this is the time for some incompatibility, until things settle.

Thanks
Laszlo



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

I see your point. I will let the patch submitter make final decision on which way they want to go.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, December 10, 2020 5:12 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> jejb@linux.ibm.com
> Cc: dovmurik@linux.vnet.ibm.com; Dov.Murik1@il.ibm.com;
> ashish.kalra@amd.com; brijesh.singh@amd.com; tobin@ibm.com;
> david.kaplan@amd.com; jon.grimm@amd.com; thomas.lendacky@amd.com;
> frankeh@us.ibm.com; Dr . David Alan Gilbert <dgilbert@redhat.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>
> Subject: Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev
> Secret area using a configuration table
> 
> Hi Jiewen,
> 
> On 12/09/20 13:02, Yao, Jiewen wrote:
> > Hi James
> > I am not sure if this solution is only for AMD SEV or it is a generic solution
> to "pass a secret to grub and let grub decrypt the disk".
> >
> > If it is only for AMD SEV, please stop reading and ignore my comment
> below.
> >
> > If it is designed to be a generic solution to pass a secret to grub and let
> grub decrypt the disk. I have some thought below:
> > Intel TDX
> (https://software.intel.com/content/www/us/en/develop/articles/intel-
> trust-domain-extensions.html) have similar feature - a TDX virtual firmware
> may do attestation and get a key from remote key server.
> > We might use same architecture to pass the secrete to grub.
> > Initially, we define an ACPI 'SVKL' table to pass the secrete in intel-tdx-
> guest-hypervisor-communication-interface
> (https://software.intel.com/content/dam/develop/external/us/en/document
> s/intel-tdx-guest-hypervisor-communication-interface.pdf), section 4.4
> storage volume key data.
> > But it is also OK if you want to use UEFI configuration table.
> >
> > If we need a common API for both AMD SEV and Intel TDX, then I
> recommend some enhancement for SevLaunchSecret.h.
> > 1) The file name (SevLaunchSecret.h) should be generic, such as
> TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'.
> Otherwise, we have to define a new GUID for 'TDX'.
> > 2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID)
> should be generic. Same reason above.
> > 3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be
> generic. Same reason above.
> > 4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should
> use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory
> location.
> > 5) The internal data structure of the secret is not defined. Is it raw binary?
> Or ASCII string password? Or DER format certificate? Or PEM format key? At
> least, we shall describe it in the header file.
> > 6) The might be a chance that a key server need input multiple keys to a
> trusted VM. How we handle this? Do we expect multiple UEFI configuration
> tables and each table support one key? or one table to support multiple
> keys?
> >
> > Would you please take a look at intel-tdx-guest-hypervisor-
> communication-interface, section 4.4 storage volume key data.
> > We defined multiple key layout, key type and key format. Please let us
> know if you have any thought.
> 
> These are several change requests. I do not disagree with them, but I
> strongly propose we implement them separately.
> 
> James's current v3 series presents a state that has been tested and
> reviewed. I'd like to commit it as-is. If for nothing else, then because
> I'd like the edk2 git commit history to have a snapshot of the work at
> this stage.
> 
> We have ample time until the next edk2 stable tag. Right after this v3
> series is committed, you guys can start generalizing it (e.g., renaming
> files and variables). Working from special case to general case is not
> uncommon. The feature need not be ready as soon as it is committed; it
> needs to be stable and externally compatible at the next stable tag.
> 
> If some changes from your list above would be incompatible with other
> software (which is also in the making, to my understanding), then I
> would request / propose that patches for those other projects be held
> back, until the generalization of the edk2 patches reaches a certain
> maturity.
> 
> Basically, I wouldn't like to do an incremental review on this series
> that introduces the above-described *amount* of changes, from v3 to v4.
> And I would like to perform a from-the-scratch review even less, on this
> series. I believe your requests have merit, I'd just like to see patches
> "on top" that implement them. We're right after the last stable tag,
> this is the time for some incompatibility, until things settle.
> 
> Thanks
> Laszlo



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

On Wed, 2020-12-09 at 12:02 +0000, Yao, Jiewen wrote:
> Hi James
> I am not sure if this solution is only for AMD SEV or it is a generic
> solution to "pass a secret to grub and let grub decrypt the disk".

Well, being open source, it's driven by the implementation, which is
AMD.  However, if you look at how it works, it's composed of a secret
discovery piece and then a packaging piece which is this part.  It's
certainly true that any discover mechanism could feed this packaging
piece, so I agree that the SecretDXE could pass up the secret from any
discovery mechanism not just SEV.

> If it is only for AMD SEV, please stop reading and ignore my comment
> below.
> 
> If it is designed to be a generic solution to pass a secret to grub
> and let grub decrypt the disk. I have some thought below:
> Intel TDX (
> https://software.intel.com/content/www/us/en/develop/articles/intel-trust-domain-extensions.html
> ) have similar feature - a TDX virtual firmware may do attestation
> and get a key from remote key server.
> We might use same architecture to pass the secrete to grub.
> Initially, we define an ACPI 'SVKL' table to pass the secrete in
> intel-tdx-guest-hypervisor-communication-interface (
> https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface.pdf
> ), section 4.4 storage volume key data.
> But it is also OK if you want to use UEFI configuration table.

From the coding point of view it's definitely easier to do a UEFI
configuration table.  If I had to insert something inside ACPI I'd be
hijacking a lot more of the guts of OVMF to achieve it.

> If we need a common API for both AMD SEV and Intel TDX, then I
> recommend some enhancement for SevLaunchSecret.h.
> 1) The file name (SevLaunchSecret.h) should be generic, such as
> TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'.
> Otherwise, we have to define a new GUID for 'TDX'.
> 2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID)
> should be generic. Same reason above.
> 3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be
> generic. Same reason above.
> 4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should
> use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory
> location.

I agree with trying to make it more generic.  what about replacing 

SEV_LAUNCH with CONFIDENTIAL_COMPUTING
SevLaunch with ConfidentialComputing

And obviously making the area a UINT64.

> 5) The internal data structure of the secret is not defined. Is it
> raw binary? Or ASCII string password? Or DER format certificate? Or
> PEM format key? At least, we shall describe it in the header file.

Since OVMF doesn't interpret the area, I didn't describe it in this
patch, but if you look at how the grub piece works:

https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00081.html

It's another GUIDed list, so all I've defined is a GUID that means
"this is a disk password".  You can define a guid for any other content
type.

> 6) The might be a chance that a key server need input multiple keys
> to a trusted VM. How we handle this? Do we expect multiple UEFI
> configuration tables and each table support one key? or one table to
> support multiple keys?

The keys would each have their own guid which would be pieced up by the
receiver.

> Would you please take a look at intel-tdx-guest-hypervisor-
> communication-interface, section 4.4 storage volume key data.
> We defined multiple key layout, key type and key format. Please let
> us know if you have any thought.

I really think the standard GUIDed form:

GUID|len|data

Works best because a GUID is big enough to define for any number of
uses and it also means we don't have to define key types or anything,
because all a new consumer has to do is define their data structure and
give it a guid.  The single uefi config table is passed through to all
the elements until it gets to one that recognizes the GUID.

James




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

Thanks. ConfidentialComputing seems a better name.

I agree with you that OVMF might not need understand the data structure.
But I am not sure if the grub is the only boot loader we want to support.

I think it might be a better idea to define the data structure clearly in OVMF.
As such, any boot loader can parse the data structure to decrypt the disk. They don’t need refer to grub.

Thank you
Yao Jiewen

> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Wednesday, December 9, 2020 11:47 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: dovmurik@linux.vnet.ibm.com; Dov.Murik1@il.ibm.com;
> ashish.kalra@amd.com; brijesh.singh@amd.com; tobin@ibm.com;
> david.kaplan@amd.com; jon.grimm@amd.com; thomas.lendacky@amd.com;
> frankeh@us.ibm.com; Dr . David Alan Gilbert <dgilbert@redhat.com>; Laszlo
> Ersek <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev
> Secret area using a configuration table
> 
> On Wed, 2020-12-09 at 12:02 +0000, Yao, Jiewen wrote:
> > Hi James
> > I am not sure if this solution is only for AMD SEV or it is a generic
> > solution to "pass a secret to grub and let grub decrypt the disk".
> 
> Well, being open source, it's driven by the implementation, which is
> AMD.  However, if you look at how it works, it's composed of a secret
> discovery piece and then a packaging piece which is this part.  It's
> certainly true that any discover mechanism could feed this packaging
> piece, so I agree that the SecretDXE could pass up the secret from any
> discovery mechanism not just SEV.
> 
> > If it is only for AMD SEV, please stop reading and ignore my comment
> > below.
> >
> > If it is designed to be a generic solution to pass a secret to grub
> > and let grub decrypt the disk. I have some thought below:
> > Intel TDX (
> > https://software.intel.com/content/www/us/en/develop/articles/intel-
> trust-domain-extensions.html
> > ) have similar feature - a TDX virtual firmware may do attestation
> > and get a key from remote key server.
> > We might use same architecture to pass the secrete to grub.
> > Initially, we define an ACPI 'SVKL' table to pass the secrete in
> > intel-tdx-guest-hypervisor-communication-interface (
> >
> https://software.intel.com/content/dam/develop/external/us/en/document
> s/intel-tdx-guest-hypervisor-communication-interface.pdf
> > ), section 4.4 storage volume key data.
> > But it is also OK if you want to use UEFI configuration table.
> 
> From the coding point of view it's definitely easier to do a UEFI
> configuration table.  If I had to insert something inside ACPI I'd be
> hijacking a lot more of the guts of OVMF to achieve it.
> 
> > If we need a common API for both AMD SEV and Intel TDX, then I
> > recommend some enhancement for SevLaunchSecret.h.
> > 1) The file name (SevLaunchSecret.h) should be generic, such as
> > TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'.
> > Otherwise, we have to define a new GUID for 'TDX'.
> > 2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID)
> > should be generic. Same reason above.
> > 3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be
> > generic. Same reason above.
> > 4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should
> > use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory
> > location.
> 
> I agree with trying to make it more generic.  what about replacing
> 
> SEV_LAUNCH with CONFIDENTIAL_COMPUTING
> SevLaunch with ConfidentialComputing
> 
> And obviously making the area a UINT64.
> 
> > 5) The internal data structure of the secret is not defined. Is it
> > raw binary? Or ASCII string password? Or DER format certificate? Or
> > PEM format key? At least, we shall describe it in the header file.
> 
> Since OVMF doesn't interpret the area, I didn't describe it in this
> patch, but if you look at how the grub piece works:
> 
> https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00081.html
> 
> It's another GUIDed list, so all I've defined is a GUID that means
> "this is a disk password".  You can define a guid for any other content
> type.
> 
> > 6) The might be a chance that a key server need input multiple keys
> > to a trusted VM. How we handle this? Do we expect multiple UEFI
> > configuration tables and each table support one key? or one table to
> > support multiple keys?
> 
> The keys would each have their own guid which would be pieced up by the
> receiver.
> 
> > Would you please take a look at intel-tdx-guest-hypervisor-
> > communication-interface, section 4.4 storage volume key data.
> > We defined multiple key layout, key type and key format. Please let
> > us know if you have any thought.
> 
> I really think the standard GUIDed form:
> 
> GUID|len|data
> 
> Works best because a GUID is big enough to define for any number of
> uses and it also means we don't have to define key types or anything,
> because all a new consumer has to do is define their data structure and
> give it a guid.  The single uefi config table is passed through to all
> the elements until it gets to one that recognizes the GUID.
> 
> James
> 



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

On Wed, 2020-12-09 at 16:33 +0000, Yao, Jiewen wrote:
> Thanks. ConfidentialComputing seems a better name.
> 
> I agree with you that OVMF might not need understand the data
> structure. But I am not sure if the grub is the only boot loader we
> want to support.

To be clear: grub is just using it to get the disk password.  I do
anticipate we'll also use it for provisioning keys directly into the
linux kernel as well, so multiple consumers were anticipated.

> I think it might be a better idea to define the data structure
> clearly in OVMF. As such, any boot loader can parse the data
> structure to decrypt the disk. They don’t need refer to grub.

I'll defer to what OVMF people want, but defining a table inside OVMF
that it doesn't actually use at all seems to be doing it at the wrong
layer.

James




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

> To be clear: grub is just using it to get the disk password.  I do
> anticipate we'll also use it for provisioning keys directly into the
> linux kernel as well, so multiple consumers were anticipated.

Would you please share more information about the GUIDed key usage, except disk password?

What is the usage of the provisioning key for kernel?

Thank you
Yao Jiewen



> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Thursday, December 10, 2020 12:39 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: dovmurik@linux.vnet.ibm.com; Dov.Murik1@il.ibm.com;
> ashish.kalra@amd.com; brijesh.singh@amd.com; tobin@ibm.com;
> david.kaplan@amd.com; jon.grimm@amd.com; thomas.lendacky@amd.com;
> frankeh@us.ibm.com; Dr . David Alan Gilbert <dgilbert@redhat.com>; Laszlo
> Ersek <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev
> Secret area using a configuration table
> 
> On Wed, 2020-12-09 at 16:33 +0000, Yao, Jiewen wrote:
> > Thanks. ConfidentialComputing seems a better name.
> >
> > I agree with you that OVMF might not need understand the data
> > structure. But I am not sure if the grub is the only boot loader we
> > want to support.
> 
> To be clear: grub is just using it to get the disk password.  I do
> anticipate we'll also use it for provisioning keys directly into the
> linux kernel as well, so multiple consumers were anticipated.
> 
> > I think it might be a better idea to define the data structure
> > clearly in OVMF. As such, any boot loader can parse the data
> > structure to decrypt the disk. They don’t need refer to grub.
> 
> I'll defer to what OVMF people want, but defining a table inside OVMF
> that it doesn't actually use at all seems to be doing it at the wrong
> layer.
> 
> James
> 



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

On Wed, 2020-12-09 at 16:51 +0000, Yao, Jiewen wrote:
> > To be clear: grub is just using it to get the disk password.  I do
> > anticipate we'll also use it for provisioning keys directly into
> > the linux kernel as well, so multiple consumers were anticipated.
> 
> Would you please share more information about the GUIDed key usage,
> except disk password?

I think the point here is I don't define it.  I only define the one
grub disk password use case.  The GUIDed table format means that anyone
can define a GUID and a data format for their use case.  Not actually
pre-specifying allows the use case to develop with the code.

> What is the usage of the provisioning key for kernel?

The usual problem is that you need an additional trusted public key in
the kernel primary keyring, so having the secret area inject a trusted
public key we can later use for things like third party module signing
and the like seems to be a good idea.

James




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

On Wed, 2020-12-09 at 07:46 -0800, James Bottomley wrote:
> On Wed, 2020-12-09 at 12:02 +0000, Yao, Jiewen wrote:
> > Would you please take a look at intel-tdx-guest-hypervisor-
> > communication-interface, section 4.4 storage volume key data.

OK, I read through the spec.

> > We defined multiple key layout, key type and key format. Please let
> > us know if you have any thought.
> 
> I really think the standard GUIDed form:
> 
> GUID|len|data
> 
> Works best because a GUID is big enough to define for any number of
> uses and it also means we don't have to define key types or anything,
> because all a new consumer has to do is define their data structure
> and give it a guid.  The single uefi config table is passed through
> to all the elements until it gets to one that recognizes the GUID.

The only other thing I would add here, is that you have indirect ACPI
tables whereas the above is direct.  I think indirect might be useful
at the low level for scatter gather injection if it has to be done for
the architecture, but I think to make it easier for the consumers above
OVMF we gather all the information into on GUIDed table with no
indirection, which makes the above GUIDed form the best description.

James




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

On 11/30/20 21:28, James Bottomley wrote:
> Now that the secret area is protected by a boot time HOB, extract its
> location details into a configuration table referenced by
> gSevLaunchSecretGuid so the boot loader or OS can locate it before a
> call to ExitBootServices().
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/OvmfPkg.dec                    |  1 +
>  OvmfPkg/AmdSev/AmdSevX64.dsc           |  1 +
>  OvmfPkg/AmdSev/AmdSevX64.fdf           |  1 +
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf | 37 ++++++++++++++++++++++++++
>  OvmfPkg/Include/Guid/SevLaunchSecret.h | 28 +++++++++++++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c   | 26 ++++++++++++++++++
>  6 files changed, 94 insertions(+)
>  create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>  create mode 100644 OvmfPkg/Include/Guid/SevLaunchSecret.h
>  create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 7d27f8e16040..8a294116efaa 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -117,6 +117,7 @@ [Guids]
>    gLinuxEfiInitrdMediaGuid              = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
>    gQemuKernelLoaderFsMediaGuid          = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
>    gGrubFileGuid                         = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
> +  gSevLaunchSecretGuid                  = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
>  
>  [Ppis]
>    # PPI whose presence in the PPI database signals that the TPM base address
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index e9c522bedad9..bb7697eb324b 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -778,6 +778,7 @@ [Components]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>    }
>  !endif
> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>    OvmfPkg/AmdSev/Grub/Grub.inf
>  !if $(BUILD_SHELL) == TRUE
>    ShellPkg/Application/Shell/Shell.inf {
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index b2656a1cf6fc..e8fd4b8c7b89 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -269,6 +269,7 @@ [FV.DXEFV]
>  !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(BUILD_SHELL) == TRUE
>  INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  !endif
> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>  INF  OvmfPkg/AmdSev/Grub/Grub.inf
>  !if $(BUILD_SHELL) == TRUE
>  INF  ShellPkg/Application/Shell/Shell.inf
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> new file mode 100644
> index 000000000000..62ab00a3d382
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> @@ -0,0 +1,37 @@
> +## @file
> +#  Sev Secret configuration Table installer
> +#
> +#  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = SecretDxe
> +  FILE_GUID                      = 6e2b9619-8810-4e9d-a177-d432bb9abeda
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = InitializeSecretDxe
> +
> +[Sources]
> +  SecretDxe.c
> +
> +[Packages]
> +  OvmfPkg/OvmfPkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Guids]
> +  gSevLaunchSecretGuid
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
> +
> +[Depex]
> +  TRUE
> diff --git a/OvmfPkg/Include/Guid/SevLaunchSecret.h b/OvmfPkg/Include/Guid/SevLaunchSecret.h
> new file mode 100644
> index 000000000000..fa5f3830bc2b
> --- /dev/null
> +++ b/OvmfPkg/Include/Guid/SevLaunchSecret.h
> @@ -0,0 +1,28 @@
> + /** @file
> +   UEFI Configuration Table for exposing the SEV Launch Secret location to UEFI
> +   applications (boot loaders).
> +
> +   Copyright (C) 2020 James Bottomley, IBM Corporation.
> +   SPDX-License-Identifier: BSD-2-Clause-Patent
> + **/
> +
> +#ifndef SEV_LAUNCH_SECRET_H_
> +#define SEV_LAUNCH_SECRET_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +#define SEV_LAUNCH_SECRET_GUID                          \
> +  { 0xadf956ad,                                         \
> +    0xe98c,                                             \
> +    0x484c,                                             \
> +    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47 }, \
> +  }
> +
> +typedef struct {
> +  UINT32 Base;
> +  UINT32 Size;
> +} SEV_LAUNCH_SECRET_LOCATION;
> +
> +extern EFI_GUID gSevLaunchSecretGuid;
> +
> +#endif // SEV_LAUNCH_SECRET_H_
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> new file mode 100644
> index 000000000000..d8cc9b00946a
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -0,0 +1,26 @@
> +/** @file
> +  SEV Secret configuration table constructor
> +
> +  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +#include <PiDxe.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Guid/SevLaunchSecret.h>
> +
> +STATIC SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = {
> +  FixedPcdGet32 (PcdSevLaunchSecretBase),
> +  FixedPcdGet32 (PcdSevLaunchSecretSize),
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +InitializeSecretDxe(
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_SYSTEM_TABLE     *SystemTable
> +  )
> +{
> +  return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid,
> +                                         &mSecretDxeTable
> +                                         );
> +}
> 

(1) The indentation is still not correct; it should be

  return gBS->InstallConfigurationTable (
                &gSevLaunchSecretGuid,
                &mSecretDxeTable
                );

note that the args are indented two spaces relative to the function
pointer field called "InstallConfigurationTable".

But, this can be fixed up at merge.

Now, please let me fetch my email; I'll go through any new comments I
may not have seen yet, and then I'll merge this.

Thanks!
Laszlo



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