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 7BC1E9415B2 for ; Mon, 9 Oct 2023 09:17:39 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=emspb9F/6pCGDfBt8zRgLZS2m5TuNg7x5//tnCG5/mk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: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=1696843058; v=1; b=EmN4YLXGfqA0sr+nrSoI5P88TiUuEm/RWcxCQGFa7gmiMs8EcgMzPa2F4cFU/SokKtAZdCuQ uhjA8I5rQd2Fg7XzcNvEjGTEIKbWYRmYF2+byOAY9ww+nWLrGLZhlkdnkgWyI7eX5QzqIG08Jdd A5iE4Av5aMlIwlzAMQRrJKTc= X-Received: by 127.0.0.2 with SMTP id icjSYY7687511xLgRZN04Bea; Mon, 09 Oct 2023 02:17:38 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.57008.1696843057349710374 for ; Mon, 09 Oct 2023 02:17:37 -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-5-rgB0Ti78MmeJaPKQl388AQ-1; Mon, 09 Oct 2023 05:17:35 -0400 X-MC-Unique: rgB0Ti78MmeJaPKQl388AQ-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 CC79629AA3B1; Mon, 9 Oct 2023 09:17:34 +0000 (UTC) X-Received: from [10.39.192.114] (unknown [10.39.192.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A86A1C37A87; Mon, 9 Oct 2023 09:17:33 +0000 (UTC) Message-ID: Date: Mon, 9 Oct 2023 11:17:32 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v5 22/28] OvmfPkg: Add MemoryProtectionConfigLib 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> From: "Laszlo Ersek" In-Reply-To: <20231009000742.1792-23-taylor.d.beebe@gmail.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 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: AwLGH2itQ9V845xnMHAMQ84Tx7686176AA= 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=EmN4YLXG; 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 02:07, Taylor Beebe wrote: > MemoryProtectionConfigLib enables parsing the fw_cfg for the > memory protection profile. >=20 > 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(+) Please point your git diff.order setting to "BaseTools/Conf/diff.order", fo= r formatting patches in the future. The resultant ordering of files in a pa= tch 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 > =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 handlers > # > NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h Seems OK. > diff --git a/OvmfPkg/Include/Library/MemoryProtectionConfigLib.h b/OvmfPk= g/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 p= rofile. > + > + 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 po= pulate. > + > + @retval EFI_SUCCESS The MM memory protection settings prof= ile was populated. > + @retval EFI_INVALID_PARAMETER MmSettings is NULL. > + @retval EFI_ABORTED The MM memory protection settings prof= ile name found in > + fw_cfg was invalid. > + @retval EFI_NOT_FOUND The MM memory protection settings prof= ile 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 pro= file was populated. > + @retval EFI_INVALID_PARAMETER DxeSettings is NULL. > + @retval EFI_ABORTED The DXE memory protection settings pro= file name found in > + fw_cfg was invalid. > + @retval EFI_NOT_FOUND The DXE memory protection settings pro= file was not found. > +**/ > +EFI_STATUS > +EFIAPI > +ParseFwCfgDxeMemoryProtectionSettings ( > + IN DXE_MEMORY_PROTECTION_SETTINGS *DxeSettings > + ); > + > +#endif Same comments as above. >=20 > diff --git a/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionCo= nfigLib.c b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionConfi= gLib.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 p= rofile. > + > + 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" > + 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 po= pulate. > + > + @retval EFI_SUCCESS The MM memory protection settings prof= ile was populated. > + @retval EFI_INVALID_PARAMETER MmSettings is NULL. > + @retval EFI_ABORTED The MM memory protection settings prof= ile name found in > + fw_cfg was invalid. > + @retval EFI_NOT_FOUND The MM memory protection settings prof= ile 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.=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_FWC= FG_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; 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 pro= file was populated. > + @retval EFI_INVALID_PARAMETER DxeSettings is NULL. > + @retval EFI_ABORTED The DXE memory protection settings pro= file name found in > + fw_cfg was invalid. > + @retval EFI_NOT_FOUND The DXE memory protection settings pro= file 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_FW= CFG_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].Settings= , sizeof (DXE_MEMORY_PROTECTION_SETTINGS)); > + return EFI_SUCCESS; > + } > + } > + > + return EFI_NOT_FOUND; > +} > diff --git a/OvmfPkg/Include/Dsc/MemoryProtectionLibraries.dsc.inc b/Ovmf= Pkg/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/S= etMemoryProtectionsLib.inf > + MemoryProtectionConfigLib|OvmfPkg/Library/MemoryProtectionConfigLib/Me= moryProtectionConfigLib.inf > =20 > [LibraryClasses.common.SMM_CORE, LibraryClasses.common.DXE_SMM_DRIVER, L= ibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.MM_STANDALON= E] > GetMemoryProtectionsLib|MdeModulePkg/Library/GetMemoryProtectionsLib/M= mGetMemoryProtectionsLib.inf > diff --git a/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionCo= nfigLib.inf b/OvmfPkg/Library/MemoryProtectionConfigLib/MemoryProtectionCon= figLib.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 p= rofile. > +# > +# 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-36E1894BC8E= F > + MODULE_TYPE =3D BASE > + VERSION_STRING =3D 1.0 > + LIBRARY_CLASS =3D MemoryProtectionConfigLib|SEC PEI_C= ORE PEIM > + > +# > +# The following information is for reference only and not required by th= e 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 (#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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-