Re: [edk2-devel] [PATCH v5 22/28] OvmfPkg: Add MemoryProtectionConfigLib

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

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.beebe@gmail.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-