On 2/7/2023 12:56 AM, Marvin Häuser 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.
>
>>
>>> 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 <mhaeu...@posteo.de> wrote:
On 7. Feb 2023, at 11:01, Ard Biesheuvel <a...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99757): https://edk2.groups.io/g/devel/message/99757
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]
-=-=-=-=-=-=-=-=-=-=-=-
