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

["Marvin Häuser" <mhaeuser@posteo.de>]

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.

> 
>> 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?).

> 
> 
>> 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)

Sorry to interject with no contribution, but for x86 platforms, our downstream fork removed the requirement that BSData and ConvMem need to have the same permissions. In fact, ideally ConvMem is just unmapped. Can this be enabled for ARM without a solution like Taylor’s? You said you added the requirement as a mitigation.

Unrelated FYI, we also removed the XP checks for code downstream and all types but ConvMem (which is unmapped or read-only, I forgot) have default permissions of RW. The reason for that is that unlike an OS, we don’t have a fully-featured VM system and especially things like mmap are absent. Thus, any data or code must first be written to the memory before it can be executed. The execute flag is added after loading the code to ensure W^X.

Best regards,
Marvin

> 
> 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
>>> 
>> 
>> 
>> 
>> 
>> 
>>