On Tue, 7 Feb 2023 at 10:16, Ard Biesheuvel <a...@kernel.org> wrote: > > On Tue, 7 Feb 2023 at 09:56, Marvin Häuser <mhaeu...@posteo.de> wrote: > > > > Hi Taylor and Ard, > > > > > On 7. Feb 2023, at 09:29, Ard Biesheuvel <a...@kernel.org> wrote: > > > > > > On Tue, 7 Feb 2023 at 02:18, Taylor Beebe <t...@taylorbeebe.com> wrote: > > >> > > >> I can't see the Bugzilla you referenced so I requested security Bugzilla > > >> access. But, yes, that's the bug to which I was referring :) > > >> > > > > > > I cannot see that bugzilla entry either. > > > > I CC’d you both. > > > > Thanks. > > I wrote that code but nobody ever involved me or mentioned that there > was anything wrong with it. > > > > > > >> Once Ard's change to add Memory Attribute Protocol support to ARM > > >> platforms is in, the change you linked may be palatable for the > > >> upstream. However, ARM platforms could run into the infinite loop I > > >> outlined if that change is pushed upstream because of the lack of > > >> per-allocated page tables and a software semaphore to prevent looping. > > >> > > > > > > I still don't see how this happens. There is an ASSERT in > > > CoreInitializeMemoryProtection() to ensure that EfiConventialMemory > > > and EfiBootServicesData have the same memory type, and I added that > > > (in commit 7eb927db3e25a) for precisely this reason, i.e., to ensure > > > that the plumbing of this feature wouldn't recurse. > > > > > > Could this be related to heap guard, perhaps? I could see how changing > > > the boundaries of allocations might trigger a split even if the old > > > and new type should have the exact same type, and perhaps we should > > > use unguarded pages for page tables. > > > > I know you meant recursing, but that might be related to the BZ, if I > > understood Taylor correctly. The only scenario I first-hand experienced > > this bug with was unloading a PE image. I don’t have the time right now to > > check the guarding page code in detail, but from what I just saw, I can > > very well imagine it can trigger the BZ bug (and thus potentially the > > recursion issue?). > > if we disregard explicit invocations of > EFI_DXE_SERVICES.SetMemorySpaceAttributes (), the typical region > transitions from EfiConventionalMemory to some other type and back, > and never directly from one type to another. So I would expect that > unloading a PE image would result in a FreePages () call with > EfiConventionalMemory as the new type. > > However, it appears that UnprotectUefiImage does not restore the > region's permissions to whatever is configured as the default for the > associated memory type, and so we end up with EfiConventialMemory > regions that lack the XP attribute. > > I'll send out a separate fix for that. We can resume the discussion on > your patch on the bugzilla at the same time. >
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99731): https://edk2.groups.io/g/devel/message/99731 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] -=-=-=-=-=-=-=-=-=-=-=-
