On 10/9/23 02:07, Taylor Beebe wrote:
> MemoryProtectionConfigLib enables parsing the fw_cfg for the
> memory protection profile.
>
> Signed-off-by: Taylor Beebe <taylor.d.be...@gmail.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> ---
> OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.c |
> 118 ++++++++++++++++++++
> OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc |
> 1 +
> OvmfPkg/Include/Library/MemoryProtectionConfigLib.h |
> 49 ++++++++
> OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.inf |
> 35 ++++++
> OvmfPkg/OvmfPkg.dec |
> 4 +
> 5 files changed, 207 insertions(+)
Please point your git diff.order setting to "BaseTools/Conf/diff.order", for
formatting patches in the future. The resultant ordering of files in a patch
makes for a more logical reading, progressing from generic declarations to
specific declarations to implementation.
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index e3861e5c1b39..126be04ca302 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -42,6 +42,10 @@ [LibraryClasses]
> #
> MemEncryptTdxLib|Include/Library/MemEncryptTdxLib.h
>
> + ## @libraryclass Declares helper functions for parsing fw_cfg for
> + # the memory protection profile strings
> + MemoryProtectionConfigLib|Include/Library/MemoryProtectionConfigLib.h
> +
> ## @libraryclass Handle TPL changes within nested interrupt handlers
> #
> NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
Seems OK.
> diff --git a/OvmfPkg/Include/Library/MemoryProtectionConfigLib.h
> b/OvmfPkg/Include/Library/MemoryProtectionConfigLib.h
> new file mode 100644
> index 000000000000..d30de58001c3
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/MemoryProtectionConfigLib.h
> @@ -0,0 +1,49 @@
> +/** @file
> + Parses the fw_cfg file for the DXE and MM memory protection settings
> profile.
> +
> + Copyright (c) Microsoft Corporation.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef MEMORY_PROTECTION_CONFIG_LIB_H_
> +#define MEMORY_PROTECTION_CONFIG_LIB_H_
> +
> +#include <Uefi.h>
> +
> +#include <Library/SetMemoryProtectionsLib.h>
> +
> +/**
> + Parses the fw_cfg file for the MM memory protection settings profile.
> +
> + @param[in] MmSettings The MM memory protection settings profile to
> populate.
> +
> + @retval EFI_SUCCESS The MM memory protection settings profile
> was populated.
> + @retval EFI_INVALID_PARAMETER MmSettings is NULL.
> + @retval EFI_ABORTED The MM memory protection settings profile
> name found in
> + fw_cfg was invalid.
> + @retval EFI_NOT_FOUND The MM memory protection settings profile
> was not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ParseFwCfgMmMemoryProtectionSettings (
> + IN MM_MEMORY_PROTECTION_SETTINGS *MmSettings
> + );
- "IN" is wrong; you certainly mean "OUT".
- Same for the @param[in] Doxygen macro; should be @param[out].
- EFI_ABORTED looks out of place. But, I'll comment more on this below.
> +
> +/**
> + Parses the fw_cfg file for the DXE memory protection settings profile.
> +
> + @param[in] DxeSettings The DXE memory protection settings profile to
> populate.
> +
> + @retval EFI_SUCCESS The DXE memory protection settings profile
> was populated.
> + @retval EFI_INVALID_PARAMETER DxeSettings is NULL.
> + @retval EFI_ABORTED The DXE memory protection settings profile
> name found in
> + fw_cfg was invalid.
> + @retval EFI_NOT_FOUND The DXE memory protection settings profile
> was not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ParseFwCfgDxeMemoryProtectionSettings (
> + IN DXE_MEMORY_PROTECTION_SETTINGS *DxeSettings
> + );
> +
> +#endif
Same comments as above.
>
> diff --git
> a/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.c
> b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.c
> new file mode 100644
> index 000000000000..b568665f407c
> --- /dev/null
> +++ b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.c
> @@ -0,0 +1,118 @@
> +/** @file
> + Parses the fw_cfg file for the DXE and MM memory protection settings
> profile.
> +
> + Copyright (c) Microsoft Corporation.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/QemuFwCfgSimpleParserLib.h>
> +#include <Library/SetMemoryProtectionsLib.h>
> +
> +#define DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE \
> + "opt/org.tianocore/DxeMemoryProtectionProfile"
> +
> +#define MM_MEMORY_PROTECTION_PROFILE_FWCFG_FILE \
> + "opt/org.tianocore/MmMemoryProtectionProfile"
> +
These string literals are used exactly once in the source. The macros
are superfluous.
> +/**
> + Parses the fw_cfg file for the MM memory protection settings profile.
> +
> + @param[in] MmSettings The MM memory protection settings profile to
> populate.
> +
> + @retval EFI_SUCCESS The MM memory protection settings profile
> was populated.
> + @retval EFI_INVALID_PARAMETER MmSettings is NULL.
> + @retval EFI_ABORTED The MM memory protection settings profile
> name found in
> + fw_cfg was invalid.
> + @retval EFI_NOT_FOUND The MM memory protection settings profile
> was not found.
> +**/
When you update the comment block in the header file, please don't
forget to keep this in sync.
> +EFI_STATUS
> +EFIAPI
> +ParseFwCfgMmMemoryProtectionSettings (
> + IN MM_MEMORY_PROTECTION_SETTINGS *MmSettings
> + )
> +{
> + CHAR8 String[100];
I suggest adding a maximum profile name length macro to one of the
library class header files in MdeModulePkg (I think?)
Furthermore (assuming the following point makes sense, which I'm unsure
about), when setting profiles, enforce the max length.
> + UINTN StringSize;
> + UINTN Index;
> +
> + if (MmSettings == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + StringSize = sizeof (String);
> +
> + if (!EFI_ERROR (QemuFwCfgParseString
> (MM_MEMORY_PROTECTION_PROFILE_FWCFG_FILE, &StringSize, String))) {
> + Index = 0;
> + do {
> + if (AsciiStriCmp (MmMemoryProtectionProfiles[Index].Name, String) ==
> 0) {
> + DEBUG ((DEBUG_INFO, "Setting MM Memory Protection Profile: %a\n",
> String));
> + break;
> + }
> + } while (++Index < MmMemoryProtectionSettingsMax);
> +
> + if (Index >= MmMemoryProtectionSettingsMax) {
> + DEBUG ((DEBUG_ERROR, "Invalid MM memory protection profile: %a\n",
> String));
> + ASSERT (Index < MmMemoryProtectionSettingsMax);
> + return EFI_ABORTED;
so IMO this should be EFI_NOT_FOUND
> + } else {
("else" after "return" is an anti-pattern)
> + CopyMem (MmSettings, &MmMemoryProtectionProfiles[Index].Settings,
> sizeof (MM_MEMORY_PROTECTION_SETTINGS));
> + return EFI_SUCCESS;
> + }
> + }
> +
> + return EFI_NOT_FOUND;
and this should just propagate the error from QemuFwCfgParseString() --
which effectively means EFI_PROTOCOL_ERROR.
Laszlo
> +}
> +
> +/**
> + Parses the fw_cfg file for the DXE memory protection settings profile.
> +
> + @param[in] DxeSettings The DXE memory protection settings profile to
> populate.
> +
> + @retval EFI_SUCCESS The DXE memory protection settings profile
> was populated.
> + @retval EFI_INVALID_PARAMETER DxeSettings is NULL.
> + @retval EFI_ABORTED The DXE memory protection settings profile
> name found in
> + fw_cfg was invalid.
> + @retval EFI_NOT_FOUND The DXE memory protection settings profile
> was not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ParseFwCfgDxeMemoryProtectionSettings (
> + IN DXE_MEMORY_PROTECTION_SETTINGS *DxeSettings
> + )
> +{
> + CHAR8 String[100];
> + UINTN StringSize;
> + UINTN Index;
> +
> + if (DxeSettings == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + StringSize = sizeof (String);
> +
> + if (!EFI_ERROR (QemuFwCfgParseString
> (DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE, &StringSize, String))) {
> + Index = 0;
> + do {
> + if (AsciiStriCmp (DxeMemoryProtectionProfiles[Index].Name, String) ==
> 0) {
> + DEBUG ((DEBUG_INFO, "Setting DXE Memory Protection Profile: %a\n",
> String));
> + break;
> + }
> + } while (++Index < DxeMemoryProtectionSettingsMax);
> +
> + if (Index >= DxeMemoryProtectionSettingsMax) {
> + DEBUG ((DEBUG_ERROR, "Invalid DXE memory protection profile: %a\n",
> String));
> + ASSERT (Index < DxeMemoryProtectionSettingsMax);
> + return EFI_ABORTED;
> + } else {
> + CopyMem (DxeSettings, &DxeMemoryProtectionProfiles[Index].Settings,
> sizeof (DXE_MEMORY_PROTECTION_SETTINGS));
> + return EFI_SUCCESS;
> + }
> + }
> +
> + return EFI_NOT_FOUND;
> +}
> diff --git a/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> b/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> index 049fdef3f0c1..fcd8ef23c5a5 100644
> --- a/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> +++ b/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc
> @@ -7,6 +7,7 @@
> #
> [LibraryClasses.common]
>
> SetMemoryProtectionsLib|MdeModulePkg/Library/SetMemoryProtectionsLib/SetMemoryProtectionsLib.inf
> +
> MemoryProtectionConfigLib|OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.inf
>
> [LibraryClasses.common.SMM_CORE, LibraryClasses.common.DXE_SMM_DRIVER,
> LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.MM_STANDALONE]
>
> GetMemoryProtectionsLib|MdeModulePkg/Library/GetMemoryProtectionsLib/MmGetMemoryProtectionsLib.inf
> diff --git
> a/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.inf
> b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.inf
> new file mode 100644
> index 000000000000..0ff431752901
> --- /dev/null
> +++ b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLib.inf
> @@ -0,0 +1,35 @@
> +## @file
> +# Parses the fw_cfg file for the DXE and MM memory protection settings
> profile.
> +#
> +# Copyright (c) Microsoft Corporation.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = MemoryProtectionConfigLib
> + FILE_GUID = 865BFF85-CC3A-43E7-82E1-36E1894BC8EF
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = MemoryProtectionConfigLib|SEC PEI_CORE
> PEIM
> +
> +#
> +# The following information is for reference only and not required by the
> build
> +# tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources]
> + MemoryProtectionConfigLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> + BaseMemoryLib
> + DebugLib
> + QemuFwCfgSimpleParserLib
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109446): https://edk2.groups.io/g/devel/message/109446
Mute This Topic: https://groups.io/mt/101843366/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
