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

["Laszlo Ersek" <lersek@redhat.com>]

On 11/12/20 01:13, James Bottomley wrote:
> This is to allow the boot loader (grub) to pick up the secret area.
> The Configuration Table simply points to the base and size (in
> physical memory) and this area is covered by a Boot time HOB, meaning
> that the secret will be freed after ExitBootServices, by which time it
> should be consumed anyway.
>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |  3 ++
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |  3 ++
>  .../SevLaunchSecret/SecretDxe/SecretDxe.inf   | 38 +++++++++++++++
>  .../SevLaunchSecret/SecretPei/SecretPei.inf   | 46 +++++++++++++++++++
>  .../SevLaunchSecret/SecretDxe/SecretDxe.c     | 29 ++++++++++++
>  .../SevLaunchSecret/SecretPei/SecretPei.c     | 26 +++++++++++
>  6 files changed, 145 insertions(+)
>  create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf
>  create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf
>  create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c
>  create mode 100644 OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c
>
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 7d3663150e..eb8cc9d60a 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -698,6 +698,7 @@
>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>  !endif
>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +  OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf
>
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -1007,6 +1008,8 @@
>    }
>  !endif
>
> +  OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf
> +
>    #
>    # TPM support
>    #
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index 1fd38b3fe2..65ee4d993b 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -146,6 +146,7 @@ INF  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>  INF  OvmfPkg/SmmAccess/SmmAccessPei.inf
>  !endif
>  INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +INF  OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf
>
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -290,6 +291,8 @@ INF  ShellPkg/Application/Shell/Shell.inf
>
>  INF MdeModulePkg/Logo/LogoDxe.inf
>
> +INF OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf
> +
>  #
>  # Network modules
>  #

(1) Please split the SecretDxe-related hunks and new files to a separate
patch.

(1.a) Patch#4 should be called

  OvmfPkg/AmdSev: assign and protect the Sev Secret area

It should contain the PEI phase-related changes.

(1.b) Patch#5 should inherit the present subject ("OvmfPkg/AmdSev:
Expose the Sev Secret area using a configuration table"), and should
contain the DXE phase-related hunks and new files. I'll comment on
patch#5 more, below.


(2) In both the DSC and the FDF files, please place the

  OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf

just above

  OvmfPkg/AmdSev/Grub/Grub.inf

(Not a functional difference, it just improves consistency between
the DSC and the FDF, plus this placement appears quite logical too.)


(3) I suggest unnesting both SecretPei and SecretDxe from the
SevLaunchSecret subdirectory, and removing that subdirectory. To me the
following pathnames seem fine:

- OvmfPkg/AmdSev/SecretPei/
- OvmfPkg/AmdSev/SecretDxe/


> diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf
> new file mode 100644
> index 0000000000..b154dcc74e
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.inf
> @@ -0,0 +1,46 @@
> +## @file
> +#  PEI support for SEV Secrets
> +#
> +#  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = SecretPei
> +  FILE_GUID                      = 45260dde-0c3c-4b41-a226-ef3803fac7d4
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = InitializeSecretPei
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 EBC
> +#

(4) Please either drop VALID_ARCHITECTURES, or restrict it to X64 only.


> +
> +[Sources]
> +  SecretPei.c
> +
> +[Packages]
> +  OvmfPkg/OvmfPkg.dec
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec

(5) MdeModulePkg seems unnecessary


> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  HobLib
> +  PeiServicesLib
> +  PeiServicesTablePointerLib
> +  PeimEntryPoint
> +  PcdLib

(6) only HobLib, PcdLib, and PeimEntryPoint appear necessary


> +
> +[FixedPcd]
> +  gSevLaunchSecretGuid.PcdSevLaunchSecretBase
> +  gSevLaunchSecretGuid.PcdSevLaunchSecretSize
> +
> +[Depex]
> +  TRUE

> diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c
> new file mode 100644
> index 0000000000..16b49792ad
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretPei/SecretPei.c
> @@ -0,0 +1,26 @@
> +/** @file
> +  SEV Secret boot time HOB placement
> +
> +  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +#include <PiPei.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PcdLib.h>

(7) From Library/, only HobLib and PcdLib look required.

After removing the other lib class headers, you may have to include
<Uefi/UefiMultiPhase.h> separately, for EfiBootServicesData.


> +
> +EFI_STATUS
> +EFIAPI
> +InitializeSecretPei (
> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> +  IN CONST EFI_PEI_SERVICES     **PeiServices
> +  )
> +{
> +  BuildMemoryAllocationHob (
> +    PcdGet32 (PcdSevLaunchSecretBase),
> +    PcdGet32 (PcdSevLaunchSecretSize),
> +    EfiBootServicesData);

(8) Please break the closing paren to a new line:

  BuildMemoryAllocationHob (
    PcdGet32 (PcdSevLaunchSecretBase),
    PcdGet32 (PcdSevLaunchSecretSize),
    EfiBootServicesData
    );

(Note that the indentation of the closing paren is not a mistake, it is
intentional.)


> +
> +  return EFI_SUCCESS;
> +}
>

> diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf
> new file mode 100644
> index 0000000000..085162e5c4
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.inf
> @@ -0,0 +1,38 @@
> +## @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
> +  UefiLib

(9) UefiLib looks superfluous


> +
> +[Guids]
> +  gSevLaunchSecretGuid
> +
> +[FixedPcd]
> +  gSevLaunchSecretGuid.PcdSevLaunchSecretBase
> +  gSevLaunchSecretGuid.PcdSevLaunchSecretSize
> +
> +[Depex]
> +  TRUE


> diff --git a/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c
> new file mode 100644
> index 0000000000..b40bbe1eb9
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/SevLaunchSecret/SecretDxe/SecretDxe.c
> @@ -0,0 +1,29 @@
> +/** @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/UefiLib.h>

(10) UefiLib looks superfluous.


> +#include <Library/UefiDriverEntryPoint.h>

(11) Entry point libraries must be listed in the INF files'
[LibraryClasses] sections, but should not be #included in the C source
files.


> +#include <Library/UefiBootServicesTableLib.h>
> +
> +struct {
> +  UINT32        base;
> +  UINT32        size;
> +} secretDxeTable = {
> +  FixedPcdGet32(PcdSevLaunchSecretBase),
> +  FixedPcdGet32(PcdSevLaunchSecretSize),
> +};
> +

(12) This is an interface between (theoretically) independent
applications, so we should place the structure definition in a dedicated
header file.

(12.a) Please move the following hunk, from patch#3:

> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 3fbf7a0ee1a4..b00f08341713 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

to patch#5 (i.e., the patch that's going to introduce SecretDxe).

(12.b) In patch #5, please create another new header file:

  OvmfPkg/Include/Guid/SevLaunchSecret.h

with the following contents (fix up the (C) notice):

> /** @file
>   UEFI Configuration Table for exposing the SEV Launch Secret location to UEFI
>   applications (boot loaders).
>
>   Copyright (C) <FIXME>
>   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_

(12.c) In the "SecretDxe.c" file, please use the following syntax, for
defining the table:

> #include <Guid/SevLaunchSecret.h>
>
> STATIC CONST SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = {
>   FixedPcdGet32 (PcdSevLaunchSecretBase),
>   FixedPcdGet32 (PcdSevLaunchSecretSize),
> };

(12.d) Note the space character just after "FixedPcdGet32", and the "m"
prefix in the global variable's name.


Back to your patch:

On 11/12/20 01:13, James Bottomley wrote:
> +EFI_STATUS
> +EFIAPI
> +InitializeSecretDxe(
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_SYSTEM_TABLE     *SystemTable
> +  )
> +{
> +  return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid,
> +                                         &secretDxeTable);
> +}

(13) There are two styles to indent this idiomatically:

(13.a) the official edk2 one:

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

- each argument *and* the closing paren on a new line,

- each argument *and* the closing paren indented 2 spaces relative to
  "InstallConfigurationTable" (not relative to "gBS")

(13.b) or the "condensed" style:

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

The only difference from (13.a) is that a new source line is only
started when we'd exceed the recommended line length (80).

Thanks,
Laszlo