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 C8290AC1074 for ; Mon, 9 Oct 2023 09:54:05 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=DxVCpRe+n97X8dWo5PZZwhIOeydei1WtOV5v46KS600=; 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=1696845244; v=1; b=TVhFdEMPgH4J35URkRXyPvtsrTO6CpLSfh7MiSdfnIF+AcmnfqMKPyHbVIfx97P0L13hIQrF 3ZjrYlDC+3BB7cDv58UHC3/2Li9tOVncIQNNE6Mm6wLPBzTvidmEmy4lDtuIZJAof1aF5ffxlwb +yKjZxpAd/hmbw3FF2Up/uEM= X-Received: by 127.0.0.2 with SMTP id wsijYY7687511xRIU97Ko8XQ; Mon, 09 Oct 2023 02:54:04 -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.57440.1696845243720168700 for ; Mon, 09 Oct 2023 02:54:03 -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-115-Xao4SyXWO5aJZDaOM3srIw-1; Mon, 09 Oct 2023 05:53:54 -0400 X-MC-Unique: Xao4SyXWO5aJZDaOM3srIw-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (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 4D14E3857B65; Mon, 9 Oct 2023 09:53:54 +0000 (UTC) X-Received: from [10.39.192.114] (unknown [10.39.192.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AA9FF215670B; Mon, 9 Oct 2023 09:53:52 +0000 (UTC) Message-ID: <344e0f63-1209-ce74-fff1-10b20a00b9bb@redhat.com> Date: Mon, 9 Oct 2023 11:53:51 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v5 23/28] OvmfPkg: Enable Choosing Memory Protection Profile via QemuCfg To: devel@edk2.groups.io, taylor.d.beebe@gmail.com Cc: Ard Biesheuvel , Jiewen Yao , Jordan Justen , Gerd Hoffmann , Rebecca Cran , Peter Grehan , =?UTF-8?Q?Corvin_K=c3=b6hne?= References: <20231009000742.1792-1-taylor.d.beebe@gmail.com> <20231009000742.1792-24-taylor.d.beebe@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20231009000742.1792-24-taylor.d.beebe@gmail.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 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: nYZwswHC777O9dTTeGWeUkOxx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=TVhFdEMP; 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: > Now that the EDK2 tree uses GetMemoryProtectionsLib to query > the platform memory protection settings, OvmfPkg can be updated > to use QemuCfg to set the entire memory protection profile instead > of just SetNxForStack. both the commit message and the subject say QemuCfg; should be QemuFwCfg perhaps. > > For example, the following will set the DXE memory protection to > the RELEASE preset. > -fw_cfg name=opt/org.tianocore/DxeMemoryProtectionProfile,string=release > > The following will set the MM memory protection to > the RELEASE preset. > -fw_cfg name=opt/org.tianocore/MmMemoryProtectionProfile,string=release > > For users of Stuart, DXE_MEMORY_PROTECTION_PROFILE=release and > MM_MEMORY_PROTECTION_PROFILE=release are equivalent to the above > examples. The lowercase "release" above does not match "Release" from patch 20 (MdeModulePkg: Add Additional Profiles to SetMemoryProtectionsLib). Which makes me notice that you used AsciiStr*i*Cmp() in patch 22. ... I think that's unfortunate; it guarantees that scripts passing in protection profile names will use inconsistent casing, so grepping for profile names will be limited (people will forget to grep case-insensitively). I'd rather be strict with profile names. > > Signed-off-by: Taylor Beebe > Cc: Ard Biesheuvel > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Gerd Hoffmann > Cc: Rebecca Cran > Cc: Peter Grehan > Cc: Corvin Köhne > --- > OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c | 21 ++++++++----- > OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c | 13 +------- > OvmfPkg/Library/PlatformInitLib/Platform.c | 15 --------- > OvmfPkg/PlatformPei/IntelTdx.c | 2 -- > OvmfPkg/PlatformPei/Platform.c | 33 +++++++------------- > OvmfPkg/TdxDxe/TdxDxe.c | 7 ++--- > OvmfPkg/Include/Library/PlatformInitLib.h | 13 -------- > OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf | 2 +- > OvmfPkg/PlatformCI/PlatformBuildLib.py | 8 +++++ > OvmfPkg/PlatformPei/PlatformPei.inf | 1 + > 10 files changed, 37 insertions(+), 78 deletions(-) This patch is way too big IMO, not by line count, but by number of actions taken. [cut] > diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h > index 57b18b94d9b8..b2468f206321 100644 > --- a/OvmfPkg/Include/Library/PlatformInitLib.h > +++ b/OvmfPkg/Include/Library/PlatformInitLib.h > @@ -32,7 +32,6 @@ typedef struct { > UINT32 Uc32Base; > UINT32 Uc32Size; > > - BOOLEAN PcdSetNxForStack; > UINT64 PcdTdxSharedBitMask; > > UINT64 PcdPciMmio64Base; > @@ -182,18 +181,6 @@ PlatformMemMapInitialization ( > IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > ); > > -/** > - * Fetch "opt/ovmf/PcdSetNxForStack" from QEMU > - * > - * @param Setting The pointer to the setting of "/opt/ovmf/PcdSetNxForStack". > - * @return EFI_SUCCESS Successfully fetch the settings. > - */ > -EFI_STATUS > -EFIAPI > -PlatformNoexecDxeInitialization ( > - IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > - ); > - > VOID > EFIAPI > PlatformMiscInitialization ( > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c > index f48bf16ae300..bc9becc4016e 100644 > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > @@ -249,21 +249,6 @@ PlatformMemMapInitialization ( > PlatformInfoHob->PcdPciIoSize = PciIoSize; > } > > -/** > - * Fetch "opt/ovmf/PcdSetNxForStack" from QEMU > - * > - * @param Setting The pointer to the setting of "/opt/ovmf/PcdSetNxForStack". > - * @return EFI_SUCCESS Successfully fetch the settings. > - */ > -EFI_STATUS > -EFIAPI > -PlatformNoexecDxeInitialization ( > - IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > - ) > -{ > - return QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack", &PlatformInfoHob->PcdSetNxForStack); > -} > - > VOID > PciExBarInitialization ( > VOID One one hand: OK, these look consistent, they keep the HOB interface and the lib header minimal and in sync. However, removing the "opt/ovmf/PcdSetNxForStack" fw_cfg knob is a compat breaking change. Do we have a plan for announcing that somehow? [cut] > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index bcd8d3a1be14..fa76ad2de202 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include > > #include "Platform.h" > > @@ -74,21 +75,6 @@ MemMapInitialization ( > ASSERT_RETURN_ERROR (PcdStatus); > } > > -STATIC > -VOID > -NoexecDxeInitialization ( > - IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > - ) > -{ > - RETURN_STATUS Status; > - > - Status = PlatformNoexecDxeInitialization (PlatformInfoHob); > - if (!RETURN_ERROR (Status)) { > - Status = PcdSetBoolS (PcdSetNxForStack, PlatformInfoHob->PcdSetNxForStack); > - ASSERT_RETURN_ERROR (Status); > - } > -} > - > static const UINT8 EmptyFdt[] = { > 0xd0, 0x0d, 0xfe, 0xed, 0x00, 0x00, 0x00, 0x48, > 0x00, 0x00, 0x00, 0x38, 0x00, 0x00, 0x00, 0x48, OK > @@ -345,13 +331,17 @@ InitializePlatform ( > > PublishPeiMemory (PlatformInfoHob); > > - DxeSettings = DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsPcd].Settings; > - MmSettings = MmMemoryProtectionProfiles[MmMemoryProtectionSettingsPcd].Settings; > - DxeSettings.StackExecutionProtectionEnabled = PcdGetBool (PcdSetNxForStack); > - QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack", &DxeSettings.StackExecutionProtectionEnabled); > + if (EFI_ERROR (ParseFwCfgDxeMemoryProtectionSettings (&DxeSettings))) { > + SetDxeMemoryProtectionSettings (NULL, DxeMemoryProtectionSettingsGrubCompat); > + } else { > + SetDxeMemoryProtectionSettings (&DxeSettings, DxeMemoryProtectionSettingsGrubCompat); > + } > > - SetDxeMemoryProtectionSettings (&DxeSettings, DxeMemoryProtectionSettingsPcd); > - SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd); > + if (EFI_ERROR (ParseFwCfgMmMemoryProtectionSettings (&MmSettings))) { > + SetMmMemoryProtectionSettings (NULL, MmMemoryProtectionSettingsOff); > + } else { > + SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsOff); > + } > > PlatformQemuUc32BaseInitialization (PlatformInfoHob); > OVMF can be built without SMM_REQUIRE; I think setting an MM profile in that case is wasteful (but minimally: confusing). What's more... > @@ -365,7 +355,6 @@ InitializePlatform ( > PeiFvInitialization (PlatformInfoHob); > MemTypeInfoInitialization (PlatformInfoHob); > MemMapInitialization (PlatformInfoHob); > - NoexecDxeInitialization (PlatformInfoHob); > } > > InstallClearCacheCallback (); ... your patch too shows that the new logic isn't truly compatible, or isn't *compatibly placed*, with the old logic; the old logic is restricted to non-S3 boot paths (in effect just the boot with full config boot path). > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 6b8442d12b2c..fbaa6bdc8ee5 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -66,6 +66,7 @@ [LibraryClasses] > CcExitLib > PlatformInitLib > SetMemoryProtectionsLib > + MemoryProtectionConfigLib > > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase Hmm. I would have expected the QemuFwCfgSimpleParserLib dependency to be removed here. However, I realize: in patch v5 11/28 ("OvmfPkg: Apply Memory Protections via SetMemoryProtectionsLib"), you do *not* add QemuFwCfgSimpleParserLib to "PlatformPei.inf", even though you introduce a new QemuFwCfgParseBool() call. So it's only fair that, when you now remove the QemuFwCfgParseBool() call, you don't remove the QemuFwCfgSimpleParserLib dependency either. But that only highlights a preexistent problem: even before your patch set, "OvmfPkg/PlatformPei" doesn't depend on QemuFwCfgSimpleParserLib! There isn't a single QemuFwCfgParse*() call in it! So that's a preexistent bug, namely from commit 96047b6663ab ("OvmfPkg/PlatformInitLib: Move functions to Platform.c", 2022-04-02). That was when the last QemuFwCfgParse*() reference was removed from OvmfPkg/PlatformPei, but the lib class header inclusion and the lib class dependency weren't cleaned up. Splendid. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109449): https://edk2.groups.io/g/devel/message/109449 Mute This Topic: https://groups.io/mt/101843367/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-