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

["Taylor Beebe" <t@taylorbeebe.com>]

On 2/7/2023 12:56 AM, Marvin Häuser 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.
 >
 >>
 >>> 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?).
 >
 >

The Project Mu change Marvin linked does solve the Bugzilla he created 
because it makes an explicit check to the attributes of the memory 
region being updated via ApplyMemoryProtectionAttributes() instead of 
relying on its type to determine if a call to SetAttributes() is 
required. However, because an explicit check is being made to the region 
attributes, the infinite loop is no longer negated by the requirement 
that Conventional and BootServicesData have the same XP setting making 
the loop likely to occur during InitializeDxeNxMemoryProtectionPolicy(). 
This does not occur on x86 platforms because a reserve of page table 
memory is allocated when CpuDxe loads and even if more page table memory 
must be allocated a global boolean is set in the driver to stop a 
potential loop.

So, the Bugzilla is not directly related to the looping issue, but 
solving it in the way Project Mu has reveals the issue.

On 2/7/2023 9:56 AM, Ard Biesheuvel wrote:
> On Tue, 7 Feb 2023 at 11:13, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>>
>> On 7. Feb 2023, at 11:01, Ard Biesheuvel <ardb@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, 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 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?
> 
>> 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)
> 
> 
> 
> 

If I understand Marvin correctly, he means that there either needs to be 
a requirement that if you change the attributes of an allocated buffer 
you must change them back before freeing, or the memory management logic 
should handle the possibility that the memory attributes may have 
changed since allocation. In my opinion introducing the Memory Attribute 
Protocol requires more attribute accounting when allocating and freeing. 
I believe the two changes linked below ensure that attributes are 
properly set.

1. 
https://github.com/microsoft/mu_basecore/blob/788c17f750323efc206cdb042a2c14a587dcf27a/MdeModulePkg/Core/Dxe/Mem/Page.c#L1570
2. 
https://github.com/microsoft/mu_basecore/blob/788c17f750323efc206cdb042a2c14a587dcf27a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L1562