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 C1332AC0E6F for ; Mon, 9 Oct 2023 07:28:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=C0PXdyFjjUcIxZXfrraEQCsAw0jLwC2l3AmYwzCQ/i0=; 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=1696836502; v=1; b=YPojRnUA6DDChohsNjH6EFYQ/DdM+Ezir1KMAsSmVTdP03tz9SlgkZk5KlvUfNkjr3BRdN8R ExJA6CUSrVz7IQUj1axbPZGta7jDKLW5ESjqfKk59DK2ZLvoa1IlEM/bQQGwwkyy8fB9G05qqzz 5e/o6LccwU7C3PU2F0BC8A28= X-Received: by 127.0.0.2 with SMTP id 3OXLYY7687511xt7J2Y0OPX7; Mon, 09 Oct 2023 00:28:22 -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.56153.1696836501606818296 for ; Mon, 09 Oct 2023 00:28:21 -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-443-7B5kp4tLNkyItFpdOrbOEw-1; Mon, 09 Oct 2023 03:28:19 -0400 X-MC-Unique: 7B5kp4tLNkyItFpdOrbOEw-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 0F7D2185A79B; Mon, 9 Oct 2023 07:28:19 +0000 (UTC) X-Received: from [10.39.192.114] (unknown [10.39.192.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6E1CB215670B; Mon, 9 Oct 2023 07:28:17 +0000 (UTC) Message-ID: Date: Mon, 9 Oct 2023 09:28:16 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v5 08/28] UefiCpuPkg: Always Set Stack Guard in MpPei Init To: devel@edk2.groups.io, taylor.d.beebe@gmail.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20231009000742.1792-1-taylor.d.beebe@gmail.com> <20231009000742.1792-9-taylor.d.beebe@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20231009000742.1792-9-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: aiOvP3ZybWKqvcwsC0ney8tzx7686176AA= 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=YPojRnUA; 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: > Memory protection is not set in PEI and ingested during and > after DXE handoff. This paradigm means that the platform cannot > reliably query the stack guard setting during MpInit. Because the > execution path of PEI consistent and no third party > code is executed, setting the stack guard in MpInit on every > boot should be fine. I don't understand this commit message at all -- I don't understand any one of those sentences. Please explain in detail. This is an intrusive change; it basically forces all platforms that use CpuMpPei to a mode with PcdCpuStackGuard=3DTRUE. That may not be consistent with what a platform wants, plus it seems to invalidate the definition of PcdCpuStackGuard. Is the problem that PcdGetBool() does not work? (I'd doubt that; PcdCpuStackGuard can only be fixed-at-build.) You could make the argument that PcdCpuStackGuard is *already* mis-used in CpuMpPei, given that the PCD's definition says ## Indicates if UEFI Stack Guard will be enabled. # If enabled, stack overflow in UEFI can be caught, preventing chaotic c= onsequences.

and "UEFI" is not PEI. But if this is your argument, then that's what the commit message should state. Thanks, Laszlo > > Signed-off-by: Taylor Beebe > Cc: Eric Dong > Cc: Ray Ni > Cc: Rahul Kumar > Cc: Gerd Hoffmann > --- > UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++----- > UefiCpuPkg/CpuMpPei/CpuPaging.c | 16 ++++++++-------- > UefiCpuPkg/CpuMpPei/CpuMpPei.h | 3 ++- > UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 1 - > 4 files changed, 13 insertions(+), 15 deletions(-) > > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPe= i.c > index b504bea3cfeb..ca0c6bdb4b21 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c > @@ -469,10 +469,6 @@ InitializeMpExceptionStackSwitchHandlers ( > EFI_STATUS Status; > UINT8 *Buffer; > > - if (!PcdGetBool (PcdCpuStackGuard)) { > - return; > - } > - > Status =3D MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); > ASSERT_EFI_ERROR (Status); > > @@ -589,7 +585,9 @@ InitializeCpuMpWorker ( > // > // Special initialization for the sake of Stack Guard > // > - InitializeMpExceptionStackSwitchHandlers (); > + if (mInitStackGuard) { > + InitializeMpExceptionStackSwitchHandlers (); > + } > > // > // Update and publish CPU BIST information > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPag= ing.c > index b7ddb0005b6f..0ab8ceeee8a6 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c > @@ -68,6 +68,8 @@ EFI_PEI_NOTIFY_DESCRIPTOR mPostMemNotifyList[] =3D { > } > }; > > +BOOLEAN mInitStackGuard =3D FALSE; > + > /** > The function will check if IA32 PAE is supported. > > @@ -532,7 +534,7 @@ SetupStackGuardPage ( > } > > /** > - Enable/setup stack guard for each processor if PcdCpuStackGuard is set= to TRUE. > + Enable/setup stack guard for each processor. > > Doing this in the memory-discovered callback is to make sure the Stack= Guard > feature to cover as most PEI code as possible. > @@ -553,7 +555,6 @@ MemoryDiscoveredPpiNotifyCallback ( > ) > { > EFI_STATUS Status; > - BOOLEAN InitStackGuard; > EDKII_MIGRATED_FV_INFO *MigratedFvInfo; > EFI_PEI_HOB_POINTERS Hob; > IA32_CR0 Cr0; > @@ -563,11 +564,10 @@ MemoryDiscoveredPpiNotifyCallback ( > // initialization later will not contain paging information and then f= ail > // the task switch (for the sake of stack switch). > // > - InitStackGuard =3D FALSE; > - Hob.Raw =3D NULL; > + Hob.Raw =3D NULL; > if (IsIa32PaeSupported ()) { > - Hob.Raw =3D GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid); > - InitStackGuard =3D PcdGetBool (PcdCpuStackGuard); > + Hob.Raw =3D GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid); > + mInitStackGuard =3D TRUE; > } > > // > @@ -575,7 +575,7 @@ MemoryDiscoveredPpiNotifyCallback ( > // is to enable paging if it is not enabled (only in 32bit mode). > // > Cr0.UintN =3D AsmReadCr0 (); > - if ((Cr0.Bits.PG =3D=3D 0) && (InitStackGuard || (Hob.Raw !=3D NULL)))= { > + if ((Cr0.Bits.PG =3D=3D 0) && (mInitStackGuard || (Hob.Raw !=3D NULL))= ) { > ASSERT (sizeof (UINTN) =3D=3D sizeof (UINT32)); > > Status =3D EnablePaePageTable (); > @@ -588,7 +588,7 @@ MemoryDiscoveredPpiNotifyCallback ( > Status =3D InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServic= es); > ASSERT_EFI_ERROR (Status); > > - if (InitStackGuard) { > + if (mInitStackGuard) { > SetupStackGuardPage (); > } > > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPe= i.h > index 1b9a94e18fdf..d0db4e480e13 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h > @@ -31,6 +31,7 @@ > #include > > extern EFI_PEI_PPI_DESCRIPTOR mPeiCpuMpPpiDesc; > +extern BOOLEAN mInitStackGuard; > > /** > This service retrieves the number of logical processor in the platform > @@ -426,7 +427,7 @@ InitializeCpuMpWorker ( > ); > > /** > - Enable/setup stack guard for each processor if PcdCpuStackGuard is set= to TRUE. > + Enable/setup stack guard for each processor. > > Doing this in the memory-discovered callback is to make sure the Stack= Guard > feature to cover as most PEI code as possible. > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMp= Pei.inf > index 865be5627e85..6a987754120a 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf > @@ -64,7 +64,6 @@ [Ppis] > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask = ## CONSUMES > - gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard = ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList = ## SOMETIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize = ## SOMETIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize = ## SOMETIMES_CONSUMES -=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 (#109438): https://edk2.groups.io/g/devel/message/109438 Mute This Topic: https://groups.io/mt/101843349/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-