> On 7. Feb 2023, at 18:56, Ard Biesheuvel <a...@kernel.org> wrote: > > On Tue, 7 Feb 2023 at 11:13, Marvin Häuser <mhaeu...@posteo.de> wrote: >> >> >> On 7. Feb 2023, at 11:01, Ard Biesheuvel <a...@kernel.org> wrote: >> >> Actually, it seems UnprotectUefiImage () is corrent under the >> assumption that all code regions have EFI_MEMORY_XP cleared by >> default. >> >> 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. >> >> >> Right. My main issue is, it’s nowhere documented that manually changed >> permissions must be restored to their default before freeing. Within >> DxeCore, this is easily done using the PCDs, but outside (say you allocate a >> trampoline buffer and then free it), you would need to manually query the >> permissions, store them, and restore later. >> > > Agreed. However, I'd still prefer to only call > SetMemorySpaceAttributes() if needed
Hmm, couldn’t there be some optimisation within the function itself? To my understanding, the memory / GCD maps should have the permission information without having to look them up with a page table walk, no? Again, just 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. > > 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’d rather have FreePages() take care of that honestly. Or do you mean as a workaround for tightened policies like Mu or AUDK? > >> I did *not* look into the implementation code in detail, but does the new >> memory permission protocol impose the same constraint implementation-wise >> and if so, is this documented anywhere? >> > > Not sure I follow you: which constraint is that? Having to reset the permissions to the type’s defaults prior to freeing. > >> PS: Fetched the wrong link in my last mail: >> https://lkml.org/lkml/2022/12/15/352 >> > > 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99758): https://edk2.groups.io/g/devel/message/99758 Mute This Topic: https://groups.io/mt/96664071/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
