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

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

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.

> 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 implemented pre-allocated pages for ARM a while back in a private repo
> but never committed it to Mu. Maybe that would also be worth committing
> and pushing upstream.
>

I'd like to understand better whether or not there is a way to avoid
the need for this, but if not, I'd be happy to review your solution.
Does the issue only exist on ARM? Does it still happen after I rewrote
the MMU library? (in 2020)

Thanks,
Ard.


> On 2/3/2023 11:58 AM, Marvin Häuser wrote:
> > Hi Taylor,
> >
> > Do you by any chance mean this bug?
> > https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544 <https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1544>
> >
> > I reported this a while ago at
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3316
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=3316>
> >
> > The Mu fix is by no means a workaround and actually fixes this issue in
> > a sane way. It should have been fixed upstream ages ago.
> >
> > Best regards,
> > Marvin
> >
>
>
>
> 
>
>