From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C142E7803D7 for ; Mon, 9 Oct 2023 09:22:48 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=7s+NqHLLifcCwRcZXRhMSklPrhTDdNyhyXg6dHn+i3g=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696843367; v=1; b=KlPgq0D6y5XqamISq/7gEG5Kn3Yzl+zwoOoaO1CYra9InFi6/H/H9fzaI/xxyCU5d9JBAOb5 Jd/UeAhxz4Lxput9V2q/Zt8mWm1BOThGjM0oIvpPv27WErjiu/Q/KpGClicjoHZDUQ3RYk9Zr3j aqopGHx92bw+fY8pb1uVdBZA= X-Received: by 127.0.0.2 with SMTP id GXpOYY7687511xdGSBJYxwMz; Mon, 09 Oct 2023 02:22:47 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.57057.1696843366631674737 for ; Mon, 09 Oct 2023 02:22:46 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-596--LoHLBSuOyKoXucLyKLd9A-1; Mon, 09 Oct 2023 05:22:42 -0400 X-MC-Unique: -LoHLBSuOyKoXucLyKLd9A-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3A1451C05142; Mon, 9 Oct 2023 09:22:42 +0000 (UTC) X-Received: from [10.39.192.114] (unknown [10.39.192.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 16911400F36; Mon, 9 Oct 2023 09:22:40 +0000 (UTC) Message-ID: <57e66cd8-1f21-ea4e-9ca5-6e87532a7c80@redhat.com> Date: Mon, 9 Oct 2023 11:22:39 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v5 22/28] OvmfPkg: Add MemoryProtectionConfigLib From: "Laszlo Ersek" To: devel@edk2.groups.io, taylor.d.beebe@gmail.com Cc: Ard Biesheuvel , Jiewen Yao , Jordan Justen , Gerd Hoffmann References: <20231009000742.1792-1-taylor.d.beebe@gmail.com> <20231009000742.1792-23-taylor.d.beebe@gmail.com> In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: FxiFg9ZDURTagZsqGprqijlrx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=KlPgq0D6; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 >> Cc: Ard Biesheuvel >> Cc: Jiewen Yao >> Cc: Jordan Justen >> Cc: Gerd Hoffmann >> --- >> 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(+) >=20 > 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 declaratio= ns to specific declarations to implementation. >=20 >> 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 >> =20 >> + ## @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 handler= s >> # >> NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h >=20 > Seems OK. >=20 >> diff --git a/OvmfPkg/Include/Library/MemoryProtectionConfigLib.h b/OvmfP= kg/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 >> + >> +#include >> + >> +/** >> + Parses the fw_cfg file for the MM memory protection settings profile. >> + >> + @param[in] MmSettings The MM memory protection settings profile to p= opulate. >> + >> + @retval EFI_SUCCESS The MM memory protection settings pro= file was populated. >> + @retval EFI_INVALID_PARAMETER MmSettings is NULL. >> + @retval EFI_ABORTED The MM memory protection settings pro= file name found in >> + fw_cfg was invalid. >> + @retval EFI_NOT_FOUND The MM memory protection settings pro= file was not found. >> +**/ >> +EFI_STATUS >> +EFIAPI >> +ParseFwCfgMmMemoryProtectionSettings ( >> + IN MM_MEMORY_PROTECTION_SETTINGS *MmSettings >> + ); >=20 > - "IN" is wrong; you certainly mean "OUT". >=20 > - Same for the @param[in] Doxygen macro; should be @param[out]. >=20 > - EFI_ABORTED looks out of place. But, I'll comment more on this below. >=20 >=20 >> + >> +/** >> + 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 pr= ofile was populated. >> + @retval EFI_INVALID_PARAMETER DxeSettings is NULL. >> + @retval EFI_ABORTED The DXE memory protection settings pr= ofile name found in >> + fw_cfg was invalid. >> + @retval EFI_NOT_FOUND The DXE memory protection settings pr= ofile was not found. >> +**/ >> +EFI_STATUS >> +EFIAPI >> +ParseFwCfgDxeMemoryProtectionSettings ( >> + IN DXE_MEMORY_PROTECTION_SETTINGS *DxeSettings >> + ); >> + >> +#endif >=20 > Same comments as above. >=20 >> >> diff --git a/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionC= onfigLib.c b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConf= igLib.c >> new file mode 100644 >> index 000000000000..b568665f407c >> --- /dev/null >> +++ b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLi= b.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 >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE \ >> + "opt/org.tianocore/DxeMemoryProtectionProfile" >> + >> +#define MM_MEMORY_PROTECTION_PROFILE_FWCFG_FILE \ >> + "opt/org.tianocore/MmMemoryProtectionProfile" >> + >=20 > These string literals are used exactly once in the source. The macros > are superfluous. >=20 >> +/** >> + Parses the fw_cfg file for the MM memory protection settings profile. >> + >> + @param[in] MmSettings The MM memory protection settings profile to p= opulate. >> + >> + @retval EFI_SUCCESS The MM memory protection settings pro= file was populated. >> + @retval EFI_INVALID_PARAMETER MmSettings is NULL. >> + @retval EFI_ABORTED The MM memory protection settings pro= file name found in >> + fw_cfg was invalid. >> + @retval EFI_NOT_FOUND The MM memory protection settings pro= file was not found. >> +**/ >=20 > When you update the comment block in the header file, please don't > forget to keep this in sync. >=20 >> +EFI_STATUS >> +EFIAPI >> +ParseFwCfgMmMemoryProtectionSettings ( >> + IN MM_MEMORY_PROTECTION_SETTINGS *MmSettings >> + ) >> +{ >> + CHAR8 String[100]; >=20 > I suggest adding a maximum profile name length macro to one of the > library class header files in MdeModulePkg (I think?) >=20 > Furthermore (assuming the following point makes sense, which I'm unsure > about), when setting profiles, enforce the max length.=20 >=20 >=20 >> + UINTN StringSize; >> + UINTN Index; >> + >> + if (MmSettings =3D=3D NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + StringSize =3D sizeof (String); >> + >> + if (!EFI_ERROR (QemuFwCfgParseString (MM_MEMORY_PROTECTION_PROFILE_FW= CFG_FILE, &StringSize, String))) { >> + Index =3D 0; >> + do { >> + if (AsciiStriCmp (MmMemoryProtectionProfiles[Index].Name, String)= =3D=3D 0) { >> + DEBUG ((DEBUG_INFO, "Setting MM Memory Protection Profile: %a\n= ", String)); >> + break; >> + } >> + } while (++Index < MmMemoryProtectionSettingsMax); >> + >> + if (Index >=3D MmMemoryProtectionSettingsMax) { >> + DEBUG ((DEBUG_ERROR, "Invalid MM memory protection profile: %a\n"= , String)); >> + ASSERT (Index < MmMemoryProtectionSettingsMax); >> + return EFI_ABORTED; >=20 > so IMO this should be EFI_NOT_FOUND >=20 >> + } else { >=20 > ("else" after "return" is an anti-pattern) >=20 >> + CopyMem (MmSettings, &MmMemoryProtectionProfiles[Index].Settings,= sizeof (MM_MEMORY_PROTECTION_SETTINGS)); >> + return EFI_SUCCESS; >> + } >> + } >> + >> + return EFI_NOT_FOUND; >=20 > 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 >=20 > Laszlo >=20 >> +} >> + >> +/** >> + 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 pr= ofile was populated. >> + @retval EFI_INVALID_PARAMETER DxeSettings is NULL. >> + @retval EFI_ABORTED The DXE memory protection settings pr= ofile name found in >> + fw_cfg was invalid. >> + @retval EFI_NOT_FOUND The DXE memory protection settings pr= ofile was not found. >> +**/ >> +EFI_STATUS >> +EFIAPI >> +ParseFwCfgDxeMemoryProtectionSettings ( >> + IN DXE_MEMORY_PROTECTION_SETTINGS *DxeSettings >> + ) >> +{ >> + CHAR8 String[100]; >> + UINTN StringSize; >> + UINTN Index; >> + >> + if (DxeSettings =3D=3D NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + StringSize =3D sizeof (String); >> + >> + if (!EFI_ERROR (QemuFwCfgParseString (DXE_MEMORY_PROTECTION_PROFILE_F= WCFG_FILE, &StringSize, String))) { >> + Index =3D 0; >> + do { >> + if (AsciiStriCmp (DxeMemoryProtectionProfiles[Index].Name, String= ) =3D=3D 0) { >> + DEBUG ((DEBUG_INFO, "Setting DXE Memory Protection Profile: %a\= n", String)); >> + break; >> + } >> + } while (++Index < DxeMemoryProtectionSettingsMax); >> + >> + if (Index >=3D DxeMemoryProtectionSettingsMax) { >> + DEBUG ((DEBUG_ERROR, "Invalid DXE memory protection profile: %a\n= ", String)); >> + ASSERT (Index < DxeMemoryProtectionSettingsMax); >> + return EFI_ABORTED; >> + } else { >> + CopyMem (DxeSettings, &DxeMemoryProtectionProfiles[Index].Setting= s, sizeof (DXE_MEMORY_PROTECTION_SETTINGS)); >> + return EFI_SUCCESS; >> + } >> + } >> + >> + return EFI_NOT_FOUND; >> +} >> diff --git a/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc b/Ovm= fPkg/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/M= emoryProtectionConfigLib.inf >> =20 >> [LibraryClasses.common.SMM_CORE, LibraryClasses.common.DXE_SMM_DRIVER, = LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.MM_STANDALO= NE] >> GetMemoryProtectionsLib|MdeModulePkg/Library/GetMemoryProtectionsLib/= MmGetMemoryProtectionsLib.inf >=20 >> diff --git a/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionC= onfigLib.inf b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionCo= nfigLib.inf >> new file mode 100644 >> index 000000000000..0ff431752901 >> --- /dev/null >> +++ b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfigLi= b.inf >> @@ -0,0 +1,35 @@ >> +## @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 >> +## >> + >> +[Defines] >> + INF_VERSION =3D 0x00010005 >> + BASE_NAME =3D MemoryProtectionConfigLib >> + FILE_GUID =3D 865BFF85-CC3A-43E7-82E1-36E1894BC8= EF >> + MODULE_TYPE =3D BASE >> + VERSION_STRING =3D 1.0 >> + LIBRARY_CLASS =3D MemoryProtectionConfigLib|SEC PEI_= CORE PEIM >> + >> +# >> +# The following information is for reference only and not required by t= he build >> +# tools. >> +# >> +# VALID_ARCHITECTURES =3D IA32 X64 ARM AARCH64 >> +# >> + >> +[Sources] >> + MemoryProtectionConfigLib.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + OvmfPkg/OvmfPkg.dec >> + >> +[LibraryClasses] >> + BaseLib >> + BaseMemoryLib >> + DebugLib >> + QemuFwCfgSimpleParserLib -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-