Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table

["Yao, Jiewen" <jiewen.yao@intel.com>]

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