Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 7 Feb 2023 at 10:16, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 7 Feb 2023 at 09:56, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >
> > Hi Taylor and Ard,
> >
> > > On 7. Feb 2023, at 09:29, Ard Biesheuvel <ardb@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.