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.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.
... 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
