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

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

On 10/9/23 11:17, Laszlo Ersek wrote:
> 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.

... correction: it can also be EFI_NOT_FOUND, of course, if the fw_cfg
file is not found at all.

Laszlo

> 
> 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 (#109447): https://edk2.groups.io/g/devel/message/109447
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]
-=-=-=-=-=-=-=-=-=-=-=-