On Thu, 15 Feb 2024 at 18:08, Oliver Smith-Denny <o...@linux.microsoft.com> wrote: > > On 2/14/2024 11:50 PM, Ard Biesheuvel wrote> On Thu, 15 Feb 2024 at > 01:34, Oliver Smith-Denny > > <o...@linux.microsoft.com> wrote >> This could also be fixed with > > rearchitecting the heap guard system to > >> respect alignment requirements and shift the guard pages inside of the > >> outer rounded allocation or by having guard pages be the runtime > >> granularity. Both of these approaches have issues, in the former, the > >> allocator of runtime memory would get an address that was not aligned > >> with the runtime granularity (the head guard would be, but not the > >> usuable address), which seems fraught with peril. > > > > This would be my preference, and I wouldn't expect huge problems with > > code expecting a certain alignment for such allocations. The 64k > > requirement is entirely to ensure that the OS does not have to guess > > how it should map 64k pages that have conflicting memory attributes > > due to being covered by two different misaligned entries in the UEFI > > memory map. > > > > This is also why this is important for the MAT and runtime services > > code/data regions: without 64k alignment, there will be a piece in the > > middle of each runtime DXE that requires both write and execute > > permissions. > > > > > > I do have a PR up to fix the misalignment bug (I was doing a CI check on > it before sending the patch when I did some further testing to discover > the guard pages pushing out the allocation size). Would you prefer that > I update that to do the guard page allocation inside the 64k allocation? > > I can certainly do that, again my concern is that the code starts > getting more complex, with more room for errors. The heap guard code now > needs to know the actual size requested by the caller, not the rounded > size, so we can see, oh, the allocation requested is 64k, so I need > another 64k region to fit the guards into, unless we already have shared > guard pages, in which case we may not need to, or one guard may be > shared and the other isn't, etc. It is doable, but I worry about the > added complexity for only a small window of protection for these runtime > memory regions. We could also say no shared guard pages for runtime > regions if you don't have runtime allocation granularity equal to the > EFI_PAGE_SIZE. > > Based on our offline conversation, I thought you were ok with the simple > approach of disable the guards for these regions, the value of > protecting these regions at boot time is not worth the additional > complexity. But, I can update my PR to put the guards inside the > allocation and we can compare the relative complexity. >
Of the two options you presented in this paragraph, I prefer the one where the allocation presented to the caller may not be aligned, but the region plus guards is. But disabling it entirely for these regions is still perfectly fine with me, especially if the remove ACPI reclaim memory from the set. Heap guard is a hardening feature, and if the implementation is too complex to reason about comfortably, I don't think we can confidently rely on it. And as far as the OS is concerned: with the MAT, the runtime DXEs are mapped in a way where the read-only regions are interleaved with the read-write regions, and the holes in between are not mapped at all (at least on Linux). IOW, there is some implicit guarding going on already. > >> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > >> index a2cd83345f5b..884734aff592 100644 > >> --- a/MdeModulePkg/MdeModulePkg.dec > >> +++ b/MdeModulePkg/MdeModulePkg.dec > >> @@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild] > >> # free pages for all of them. The page allocation for the type related > >> to > >> # cleared bits keeps the same as ususal. > >> # > >> + # The heap guard system only supports guarding EfiRuntimeServicesCode, > >> EfiRuntimeServicesData, > >> + # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems > >> that have > > > > I looked at the EFI spec again, and EfiACPIReclaimMemory is not > > actually listed as a memory type that has this 64k alignment > > requirement. This makes sense, given that this memory type has no > > significance to the firmware itself, only to the OS. OTOH, reserved > > memory does appear there. > > > > So I suggest we fix that first, and then drop any mention of > > EfiACPIReclaimMemory from this patch. At least we'll have heap guard > > coverage for ACPI table allocations on arm64 going forward. > > > > The logic in question was added in 2007 in commit 28a00297189c, so > > this was probably the rule on Itanium, but that support is long gone. > > > > Thanks for looking this up. I'll update either this patch or the unsent > patch I have depending on the direction we go. > Let's go with this approach. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115529): https://edk2.groups.io/g/devel/message/115529 Mute This Topic: https://groups.io/mt/104364784/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
