From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.8945.1685451096404199618 for ; Tue, 30 May 2023 05:51:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=SwRv5lZ4; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id C480B62FA2 for ; Tue, 30 May 2023 12:51:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3B0F1C4339C for ; Tue, 30 May 2023 12:51:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685451095; bh=EDqb5uHviZkEDu0Qpzgu0tgz93mVzdcrMoX3ZNMMogA=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=SwRv5lZ41g9Cw4XEhRUWy4RruUoh5viFHhDKcd6t4Ssu0RhKLzxQ8OkGEWAZXGv6G 0jnOKTYly44EQinkzDb5mPeDLY/AlcEdbRlK1IC9/+z4klUrFk/CM5oz60QJkCdGW9 1qF895tezaw1Q3X4kekXRrTGMqhv+w5Pwz7/2rfjFuvIWRSdwfDxEB7PmQ7D/aEfh3 C/xrfFLCrfgc2RfzfIGWhgGTV29S0r1Fg9DcVnccHd+61Vt2e7eYtJPZHcJX0CvOVr GDFAo1nscJxpD1cIe1SosAeDUyCh1LcmZ1cgpMsZtcygke8fYp3kghzvhMuU2J+w6d s6+yE2iYQhKLQ== Received: by mail-lf1-f53.google.com with SMTP id 2adb3069b0e04-4f3edc05aa5so4832567e87.3 for ; Tue, 30 May 2023 05:51:35 -0700 (PDT) X-Gm-Message-State: AC+VfDxiN6DA9fzGETRUQuYffv3rYzSDHNPkVc8y2FRKLs5OAsQTOQgq XFdoyVqHfeHoG9sLbXZklu7X7Kl+ASTokXDOWvQ= X-Google-Smtp-Source: ACHHUZ4moPgWbQwEBPsBOmBYsjkaX2I6URIb7+ktIWuVkaGFKcH8ceW6uwkrr6LDJgk6iFaMhcfeu1wcPYs3izUrg94= X-Received: by 2002:a2e:a305:0:b0:2a7:748c:1eef with SMTP id l5-20020a2ea305000000b002a7748c1eefmr835416lje.38.1685451093230; Tue, 30 May 2023 05:51:33 -0700 (PDT) MIME-Version: 1.0 References: <20230525143041.1172989-1-ardb@kernel.org> <20230525143041.1172989-10-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 30 May 2023 14:51:21 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX To: "Tan, Dun" Cc: "Ni, Ray" , "devel@edk2.groups.io" , "Yao, Jiewen" , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , "Bi, Dandan" , "Gao, Liming" , "Kinney, Michael D" , Leif Lindholm , Sunil V L , "Warkentin, Andrei" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 30 May 2023 at 12:25, Tan, Dun wrote: > > Ray, > I think using MemoryAttribute PPI also looks good for X64 DxeIpl. > The only question that comes to my mind is the AMD sev feature. Since the= MemoryAttribute can't handle the AMD sev feature requirements(remapping gh= cb range from non-1:1 mapping to 1:1-mapping), we may need to find an appro= priate place to remap the Ghcb range. > To me, the structure of that code seems entirely wrong, to be honest. If SEV requires an additional step to be performed at end of PEI, it should be done in a separate PEIM that registers a notification on that signal. However, I must admit that the X64 PEI logic is confusing to me, so I may have missed something here. It seems to me that creating an entirely new set of page tables in DxeIpl is both redundant (as PEI already executes in long mode, and therefore uses page tables) and undesirable, as it remaps the entire address space read/write/execute without any awareness of what those regions may contain. Would it be possible to remove this logic from DxeIpl, and set up the page tables properly during PEI? > -----Original Message----- > From: Ni, Ray > Sent: Tuesday, May 30, 2023 3:19 PM > To: Ard Biesheuvel ; devel@edk2.groups.io; Tan, Dun > Cc: Yao, Jiewen ; Gerd Hoffmann = ; Taylor Beebe ; Oliver Smith-Denny ; Bi, Dandan ; Gao, Liming = ; Kinney, Michael D ; Leif Lindholm ; Sunil V L ; Warkentin, Andrei <= andrei.warkentin@intel.com> > Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute = PPI to remap the stack NX > > Looks good. > > @Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what ope= ns will there be for X64 DxeIpl? > > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Thursday, May 25, 2023 10:31 PM > > To: devel@edk2.groups.io > > Cc: Ard Biesheuvel ; Ni, Ray ; Yao, > > Jiewen ; Gerd Hoffmann ; > > Taylor Beebe ; Oliver Smith-Denny > > ; Bi, Dandan ; Gao, Liming > > ; Kinney, Michael D > > ; Leif Lindholm > > ; Sunil V L ; > > Warkentin, Andrei > > Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute > > PPI to remap the stack NX > > > > If the associated PCD is set to TRUE, use the memory attribute PPI to > > remap the stack non-executable. This provides a generic method for > > doing so, which will be used by ARM and AArch64 as well once they move > > to the generic DxeIpl handoff implementation. > > > > Signed-off-by: Ard Biesheuvel > > --- > > MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29 ++++++++++++++++++-- > > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++- > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > > b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > > index a0f85ebea56e6cba..22caabb02840ba88 100644 > > --- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > > @@ -2,12 +2,15 @@ > > Generic version of arch-specific functionality for DxeLoad. > > > > > > > > Copyright (c) 2006 - 2018, Intel Corporation. All rights > > reserved.
> > > > +Copyright (c) 2023, Google, LLC. All rights reserved.
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > > > **/ > > > > > > > > #include "DxeIpl.h" > > > > > > > > +#include > > > > + > > > > /** > > > > Transfers control to DxeCore. > > > > > > > > @@ -25,9 +28,10 @@ HandOffToDxeCore ( > > IN EFI_PEI_HOB_POINTERS HobList > > > > ) > > > > { > > > > - VOID *BaseOfStack; > > > > - VOID *TopOfStack; > > > > - EFI_STATUS Status; > > > > + VOID *BaseOfStack; > > > > + VOID *TopOfStack; > > > > + EFI_STATUS Status; > > > > + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi; > > > > > > > > // > > > > // Allocate 128KB for the Stack > > > > @@ -35,6 +39,25 @@ HandOffToDxeCore ( > > BaseOfStack =3D AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE)); > > > > ASSERT (BaseOfStack !=3D NULL); > > > > > > > > + if (PcdGetBool (PcdSetNxForStack)) { > > > > + Status =3D PeiServicesLocatePpi ( > > > > + &gEdkiiMemoryAttributePpiGuid, > > > > + 0, > > > > + NULL, > > > > + (VOID **)&MemoryPpi > > > > + ); > > > > + ASSERT_EFI_ERROR (Status); > > > > + > > > > + Status =3D MemoryPpi->SetPermissions ( > > > > + MemoryPpi, > > > > + (UINTN)BaseOfStack, > > > > + STACK_SIZE, > > > > + EFI_MEMORY_XP, > > > > + 0 > > > > + ); > > > > + ASSERT_EFI_ERROR (Status); > > > > + } > > > > + > > > > // > > > > // Compute the top of the stack we were allocated. Pre-allocate a > > UINTN > > > > // for safety. > > > > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > > index 60c998be6c1bad01..7126a96d8378d1f8 100644 > > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > > @@ -91,6 +91,7 @@ [Ppis] > > gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES > > > > gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES > > > > gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES # Con= sumed > > on firmware update boot path > > > > + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES > > > > > > > > [Guids] > > > > ## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation" > > > > @@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64] > > gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize = ## CONSUMES > > > > > > > > [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64] > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## > > SOMETIMES_CONSUMES > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## > > SOMETIMES_CONSUMES > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## > > SOMETIMES_CONSUMES > > > > > > > > +[Pcd] > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## > > SOMETIMES_CONSUMES > > > > + > > > > [Depex] > > > > gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid > > > > > > > > -- > > 2.39.2 >