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 74B2B941288 for ; Mon, 9 Oct 2023 08:19:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=NGf/YaoObeS8g8prbySXCNJi6d7wb4BcbSAFizmGaAw=; 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=1696839572; v=1; b=ocYysrM0SRYBoKKj8hFJhiPGyDFmRfFvXl3yQnkfrWG+CKIbQQQmp1zWlE5GHUgla9c6l2lb K+hsVSKQzjrB0QMz0LgeHLn+jSmx8LMyszu6ei4G3DifMyrPYWzB7HLInaqsPyBHGyDS+sK/jQb fTPcKIcdQjYd73j0ZENjk80c= X-Received: by 127.0.0.2 with SMTP id CndJYY7687511xBDic1SVhYu; Mon, 09 Oct 2023 01:19:32 -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.web10.56744.1696839571382456276 for ; Mon, 09 Oct 2023 01:19:31 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-511-DzlyLHiXM9ipkL0M2AdHNQ-1; Mon, 09 Oct 2023 04:19:26 -0400 X-MC-Unique: DzlyLHiXM9ipkL0M2AdHNQ-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 F2A59810BDC; Mon, 9 Oct 2023 08:19:25 +0000 (UTC) X-Received: from [10.39.192.114] (unknown [10.39.192.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CEEF22027045; Mon, 9 Oct 2023 08:19:24 +0000 (UTC) Message-ID: Date: Mon, 9 Oct 2023 10:19:23 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v5 11/28] OvmfPkg: Apply Memory Protections via SetMemoryProtectionsLib 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-12-taylor.d.beebe@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20231009000742.1792-12-taylor.d.beebe@gmail.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 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: HTPWGJINN5lgs6vyfmyJWxASx7686176AA= 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=ocYysrM0; 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: > Use SetMemoryProtectionsLib to set the memory protections for > the platform in both normal and PEI-less boot. The protections > set are equivalent to the PCD settings and the ability to set > NxForStack via QemuCfg is preserved. Once the transition to use > SetMemoryProtectionsLib and GetMemoryProtectionsLib is complete > in the rest of EDK2, the mechanics of setting protections in > OvmfPkg will be updated and the memory protection PCDs will > be deleted. >=20 > Signed-off-by: Taylor Beebe > Cc: Ard Biesheuvel > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Gerd Hoffmann > --- > OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c | 15 +++++++++++= ++-- > OvmfPkg/PlatformPei/Platform.c | 15 +++++++++++= ++-- > OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf | 3 +++ > OvmfPkg/PlatformPei/PlatformPei.inf | 1 + > 4 files changed, 30 insertions(+), 4 deletions(-) This should be two patches; the audiences for both modules (PeilessStartupLib vs. PlatformPei) are nearly distinct. I surely don't care for PeilessStartupLib. >=20 > diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg= /Library/PeilessStartupLib/PeilessStartup.c > index 1632a2317718..cf645aad3246 100644 > --- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c > +++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c > @@ -14,10 +14,13 @@ > #include > #include > #include > +#include > #include > #include > #include > #include > +#include > +#include > #include > #include > #include > @@ -42,7 +45,9 @@ InitializePlatform ( > EFI_HOB_PLATFORM_INFO *PlatformInfoHob > ) > { > - VOID *VariableStore; > + VOID *VariableStore; > + DXE_MEMORY_PROTECTION_SETTINGS DxeSettings; > + MM_MEMORY_PROTECTION_SETTINGS MmSettings; > =20 > DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n")); > PlatformDebugDumpCmos (); > @@ -104,7 +109,13 @@ InitializePlatform ( > =20 > PlatformMemMapInitialization (PlatformInfoHob); > =20 > - PlatformNoexecDxeInitialization (PlatformInfoHob); > + DxeSettings =3D DxeMemoryProtectionPro= files[DxeMemoryProtectionSettingsPcd].Settings; > + MmSettings =3D MmMemoryProtectionProf= iles[MmMemoryProtectionSettingsPcd].Settings; > + DxeSettings.StackExecutionProtectionEnabled =3D PcdGetBool (PcdSetNxFo= rStack); > + QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack", &DxeSettings.StackExe= cutionProtectionEnabled); > + > + SetDxeMemoryProtectionSettings (&DxeSettings, DxeMemoryProtectionSetti= ngsPcd); > + SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettings= Pcd); > =20 > if (TdIsEnabled ()) { > PlatformInfoHob->PcdConfidentialComputingGuestAttr =3D CCAttrIntelTd= x; > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platfor= m.c > index f5dc41c3a8c4..bcd8d3a1be14 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -38,6 +38,7 @@ > #include > #include > #include > +#include > =20 > #include "Platform.h" > =20 > @@ -304,8 +305,10 @@ InitializePlatform ( > IN CONST EFI_PEI_SERVICES **PeiServices > ) > { > - EFI_HOB_PLATFORM_INFO *PlatformInfoHob; > - EFI_STATUS Status; > + EFI_HOB_PLATFORM_INFO *PlatformInfoHob; > + EFI_STATUS Status; > + DXE_MEMORY_PROTECTION_SETTINGS DxeSettings; > + MM_MEMORY_PROTECTION_SETTINGS MmSettings; > =20 > DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n")); > PlatformInfoHob =3D BuildPlatformInfoHob (); Can you check, in the OVMF debug log, the before-after temp stack usage? My build of OVMF logs: Temp Stack : BaseAddress=3D0x818000 Length=3D0x8000 Temp Heap : BaseAddress=3D0x810000 Length=3D0x8000 Total temporary memory: 65536 bytes. temporary memory stack ever used: 30112 bytes. temporary memory heap used for HobList: 7600 bytes. I wonder if your change above makes a difference for stack usage here. Point of interest: commit d41fd8e839a3 ("OvmfPkg: restore temporary SEC/PEI RAM size to 64KB", 2017-11-17). > @@ -342,6 +345,14 @@ InitializePlatform ( > =20 > PublishPeiMemory (PlatformInfoHob); > =20 > + DxeSettings =3D DxeMemoryProtectionPro= files[DxeMemoryProtectionSettingsPcd].Settings; > + MmSettings =3D MmMemoryProtectionProf= iles[MmMemoryProtectionSettingsPcd].Settings; Can you perhaps rearrange the source code somehow in order to prevent uncrustify from turning these assignments into 110+ charater long lines? But more importantly, these are structure assignments, and (I think) structure assignments are (still) forbidden in edk2, because they can produce calls to compiler-intrinsic helper functions. So, please use CopyMem(). (And that should solve the line-too-long issue as well!) > + DxeSettings.StackExecutionProtectionEnabled =3D PcdGetBool (PcdSetNxFo= rStack); > + QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack", &DxeSettings.StackExe= cutionProtectionEnabled); > + > + SetDxeMemoryProtectionSettings (&DxeSettings, DxeMemoryProtectionSetti= ngsPcd); > + SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettings= Pcd); > + > PlatformQemuUc32BaseInitialization (PlatformInfoHob); > =20 > InitializeRamRegions (PlatformInfoHob); I don't like the placement of this logic (or minimally, I don't understand it). This logic is supposed to be a superset of NoexecDxeInitialization(), as the latter is what currently consumes the "opt/ovmf/PcdSetNxForStack" fw_cfg file, and sets PcdSetNxForStack accordingly. But NoexecDxeInitialization() is restricted to PlatformInfoHob->BootMode !=3D BOOT_ON_S3_RESUME whereas the new logic appears to run regardless of boot mode. Did you test this series with ACPI S3 suspend/resume, both with and without SMM_REQUIRE? During S3 resume, there's not going to be a DXE phase, and so SMM is not loaded yet again (it just survives from the first, cold, boot). Therefore anything we expose (such as a HOB) that controls only DXE or SMM is irrelevant during S3. If there is at least one a setting in "DxeSettings" that *does* make a difference even during S3 resume (stack guard?), then I'd argue that the DXE settings are incorrectly structured / named. Laszlo > diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf b/Ov= mfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf > index 585d50463748..f0a8a5a56df4 100644 > --- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf > +++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf > @@ -56,6 +56,8 @@ [LibraryClasses] > PrePiLib > QemuFwCfgLib > PlatformInitLib > + SetMemoryProtectionsLib > + QemuFwCfgSimpleParserLib > =20 > [Guids] > gEfiHobMemoryAllocModuleGuid > @@ -81,6 +83,7 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## SOMET= IMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask = ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask = ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack = ## CONSUMES > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize > gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootSupported > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/Pl= atformPei.inf > index 3934aeed9514..6b8442d12b2c 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -65,6 +65,7 @@ [LibraryClasses] > PcdLib > CcExitLib > PlatformInitLib > + SetMemoryProtectionsLib > =20 > [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase -=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 (#109443): https://edk2.groups.io/g/devel/message/109443 Mute This Topic: https://groups.io/mt/101843353/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-