[edk2-devel] [PATCH 4/4] OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table
Laszlo Ersek
lersek at redhat.com
Tue Nov 17 00:12:38 UTC 2020
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 at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67623): https://edk2.groups.io/g/devel/message/67623
Mute This Topic: https://groups.io/mt/78198621/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list