From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web10.91046.1675793959876152657 for ; Tue, 07 Feb 2023 10:19:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=ZpCRWP7U; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 22FE9240522 for ; Tue, 7 Feb 2023 19:19:17 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1675793958; bh=tkPewdEwGP+3LucYywEmxtN/j7nAAHkK8jeVDLyayLU=; h=From:Subject:Date:Cc:To:From; b=ZpCRWP7UhRo1px0pyXF9paPjcHP22hfJfZmpmd7jJOV+WOMLLUj39heVTL7TCAAZo O3wBIjIfzzRj5wc2aF19I7fh7y2x1T45SNGz2DgyaQ6PkmDEWoi5R4ThqQk2nTBm9P izprQL0+pMTnGipI0t0Vdfdoxs+uym8v6DOAZRiNm7QdHwZDf1mKLEfHbBpeg83RYd ob04bdsUk5faW52xlMavYFooo4HHbFsgNn6RRQzoo2KRzZ0A01401BEr9wsAZY31n0 HsZIKWCeRK9tfh2l4Wy7z8k5kP/kwsgakxckh2s+VYFldnpMRcrEL8NN30MfXYu2KZ akEo3F8NBnw7Q== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PBBGs2XPrz6tmP; Tue, 7 Feb 2023 19:19:17 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Date: Tue, 7 Feb 2023 18:19:16 +0000 Message-Id: References: Cc: devel@edk2.groups.io, t@taylorbeebe.com In-Reply-To: To: Ard Biesheuvel Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 7. Feb 2023, at 18:56, Ard Biesheuvel wrote: >=20 > =EF=BB=BFOn Tue, 7 Feb 2023 at 11:13, Marvin H=C3=A4user wrote: >>=20 >>=20 >> On 7. Feb 2023, at 11:01, Ard Biesheuvel wrote: >>=20 >> Actually, it seems UnprotectUefiImage () is corrent under the >> assumption that all code regions have EFI_MEMORY_XP cleared by >> default. >>=20 >> However, if you redefine the policy to set EFI_MEMORY_XP on code >> regions by default, and only permit execution after remapping the code >> read-only explicitly, and only then clearing EFI_MEMORY_XP, that >> routine should revert the region to EFI_MEMORY_XP. But given the >> existing ASSERT()s on having EFI_MEMORY_XP cleared for all code >> regions, the code as it is currently is not incorrect. >>=20 >>=20 >> Right. My main issue is, it=E2=80=99s nowhere documented that manually ch= anged permissions must be restored to their default before freeing. Within D= xeCore, this is easily done using the PCDs, but outside (say you allocate a t= rampoline buffer and then free it), you would need to manually query the per= missions, store them, and restore later. >>=20 >=20 > Agreed. However, I'd still prefer to only call > SetMemorySpaceAttributes() if needed Hmm, couldn=E2=80=99t there be some optimisation within the function itself?= To my understanding, the memory / GCD maps should have the permission infor= mation without having to look them up with a page table walk, no? Again, jus= t talking high-level here, ignoring any low-level details. > , and setting the same attributes > on the entire image allocation at least permits us to double check > whether the new attributes are already set on a region, and avoid the > call if that is the case. >=20 > Perhaps we should just set EFI_MEMORY_XP on all images regardless of > the policy - the memory should no longer be executable anyway, and > what we currently do is remap the entire region RWX after it has > executed, which is kind of nasty. I=E2=80=99d rather have FreePages() take care of that honestly. Or do you me= an as a workaround for tightened policies like Mu or AUDK? >=20 >> I did *not* look into the implementation code in detail, but does the new= memory permission protocol impose the same constraint implementation-wise a= nd if so, is this documented anywhere? >>=20 >=20 > Not sure I follow you: which constraint is that? Having to reset the permissions to the type=E2=80=99s defaults prior to free= ing. >=20 >> PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12= /15/352 >>=20 >=20 > Yeah saw that. I'm hoping to get that in for v6.4 as we missed v6.3 by > now. (I did take his patch that adds the definition of the EFI memory > attribute protocol only, as I need it for EFI zboot) :) Best regards, Marvin=20=