[edk2-devel] [PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
Michael D Kinney
michael.d.kinney at intel.com
Mon May 1 17:49:01 UTC 2023
Hi,
These UEFI Memory Map, GCD, and Page Table interactions can be complex and I
agree there are some UEFI/PI spec clarifications that may help.
Ray hosts a TianoCore design meeting when needed. Do you think a meeting with
an open discussion on these topics would help, or do we prefer to continue with
email discussions?
Thanks,
Mike
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Oliver
> Smith-Denny
> Sent: Monday, May 1, 2023 10:04 AM
> To: Ard Biesheuvel <ardb at kernel.org>
> Cc: devel at edk2.groups.io; Leif Lindholm <quic_llindhol at quicinc.com>; Ard
> Biesheuvel <ardb+tianocore at kernel.org>; Sami Mujawar
> <sami.mujawar at arm.com>; Michael Kubacki
> <mikuback at linux.microsoft.com>; Sean Brogan
> <sean.brogan at microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD
> Capabilities With Page Table Attributes
>
> On 5/1/2023 6:02 AM, Ard Biesheuvel wrote:
> > On Wed, 26 Apr 2023 at 02:09, Oliver Smith-Denny
> > <osde at linux.microsoft.com> wrote:
> >>
> >> When ArmPkg's CpuDxe driver initializes, it attempts to sync the
> >> GCD with the page table. However, unlike when the UefiCpuPkg's
> >> CpuDxe initializes, the Arm version does not update the GCD
> >> capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set
> >> the capabilities to be the existing page table attributes for
> >> this range, but the UefiCpuPkg CpuDxe sets the above attributes
> >> as they are software constructs, possible to set for any memory
> >> hardware).
> >>
> >> As a result, when the GCD attributes are attempted to be set
> >> against the old GCD capabilities, attributes that are set in the
> >> page table can get lost because the new attributes are not in the
> >> old GCD capabilities (and yet they are already set in the page
> >> table) meaning that the attempted sync between the GCD and the
> >> page table was a failure and drivers querying one vs the other
> >> will see different state. This can lead to RWX memory regions
> >> even with the no-execute policy set, because core drivers (such
> >> as NonDiscoverablePciDeviceIo, to be fixed up in a later patchset)
> >> allocate pages, query the GCD attributes, attempt to set a new
> >> cache attribute and end up clearing the XP bit in the page table
> >> because the GCD attributes do not have XP set.
> >>
> >> This patch follows the UefiCpuPkg pattern and adds
> >> EFI_MEMORY_[RO|RP|XP] to the GCD capabilities during CpuDxe
> >> initialization. This ensures that memory regions which already have
> >> these attributes set get them set in the GCD attributes, properly
> >> syncing between the GCD and the page table.
> >>
> >> This mitigates the issue seen, however, additional investigations
> >> into setting the GCD attributes earlier and maintaining a better
> >> sync between the GCD and the page table are being done.
> >>
> >> Feedback on this proposal is greatly appreciated, particularly
> >> any pitfalls or more architectural solutions to issues seen
> >> with syncing the GCD and the page table.
> >>
> >
> > I think this is conceptually correct. However, I've played with this
> > in the past, and IIRC, these attributes are inherited by the EFI
> > memory map too, and not updated when allocations are made. This means
> > that all code and data regions will be listed as RP, RO and XP in the
> > EFI memory map, which is going to confuse Linux at the very least, and
> > potentially other OSes as well.
>
> Thanks for the detailed response, I appreciate it!
> A clarification here: my patch follows what the x86 code does (see
> https://github.com/tianocore/edk2/blob/56e9828380b7425678a080bd3a08e
> 7c741af67ba/UefiCpuPkg/CpuDxe/CpuPageTable.c#LL994C1-L1077C27),
> where the capabilities of each region are updated to include RP, RO, and
> XP. However, the attributes of each region are only updated to include
> these flags if the page table has the attributes set for this region.
> I.e. we are only allowing for the possibility of these bits to be
> set, but not actually setting them in the attributes unless the page
> table already has them set. So, at the very least, we should be
> matching what x86 does (so OS's should not be broken), unless
> the other discrepancies in the x86 and Arm memory attribute handling
> cause issue.
>
> The main thing my change is intended to address is the case where
> code that uses the GCD instead of the page table (such as the
> NonDiscoverablePciDeviceIo driver I mentioned) attempts to set
> a CPU memory attribute (such as UC) and ends up clearing the XP bit
> that was set in the page table, but wasn't in the GCD (because it was
> never properly synced with the page table). This could also be solved
> by going to each caller and updating it to set the GCD capabilities
> before it sets the GCD attributes (and perhaps they should be, already).
> However, this puts the burden of maintaining memory protections such
> as XP on each caller, instead of handling it at a core level (now,
> there are of course complete design changes that could make this more
> effective, but this was an attempt at a simple first step).
>
> My understanding is that the EFI memory map is built on demand by calls
> to GetMemoryMap from the page descriptor list, which is updated
> thoughout bootservices, i.e. that it isn't pulling from the GCD. I will
> go back and trace through this logic again.
>
> >
> > Generally, the EFI and PI specs are very vague when it comes to
> > defining whether memory region attributes apply to the memory region
> > itself ("this memory can be mapped as read-only or non-exec") or its
> > contents ("these pages can be mapped read-only because the code does
> > not expect to be able to write to it").
> >
> > Before we have some clarification and some rigid wording in the spec
> > as to what these attributes actually mean for non-allocated regions vs
> > allocated ones, I don't think we can use them like this.
> > Alternatively, we could update the GCD core so these attributes are
> > not inherited by the EFI memory map.
> >
> > Another potential concern is fragmentation: the GCD memory map has
> > very few entries usually under the current usage model, but keeping
> > track of all memory permission attributes is going to inflate its size
> > substantially.
>
> I think this should not change the size of the GCD, right? It is not
> tracking any more regions than it already was. It simply is updating
> the capabilities of each region it was already tracking (and possibly
> the attributes if the page table had RO, RP, or XP set). I agree that
> not tracking each range of memory is critical, we have seen issues when
> non-existent memory was getting tracked.
>
> >
> > Also, including EFI_MEMORY_RP only makes sense if we implement a way
> > to apply it, which ArmCpuDxe currently does not provide.
>
> Thanks for pointing this out, I hadn't traced and seen that RP is not
> implemented. If this moves forward, I can drop it (and with a comment
> why it is different).
>
> >
> >
> >> PR: https://github.com/tianocore/edk2/pull/4311
> >> Personal branch: https://github.com/os-
> d/edk2/tree/osde/sync_aarch64_gcd_capabilities_v1
> >>
> >> Cc: Leif Lindholm <quic_llindhol at quicinc.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
> >> Cc: Sami Mujawar <sami.mujawar at arm.com>
> >> Cc: Michael Kubacki <mikuback at linux.microsoft.com>
> >> Cc: Sean Brogan <sean.brogan at microsoft.com>
> >>
> >> Signed-off-by: Oliver Smith-Denny <osde at linux.microsoft.com>
> >> ---
> >> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 55
> +++++++++++++++++---
> >> 1 file changed, 49 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> index 2e73719dce04..3ef0380e084f 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> >> @@ -90,6 +90,7 @@ SetGcdMemorySpaceAttributes (
> >> UINTN EndIndex;
> >> EFI_PHYSICAL_ADDRESS RegionStart;
> >> UINT64 RegionLength;
> >> + UINT64 Capabilities;
> >>
> >> DEBUG ((
> >> DEBUG_GCD,
> >> @@ -146,14 +147,56 @@ SetGcdMemorySpaceAttributes (
> >> RegionLength = MemorySpaceMap[Index].BaseAddress +
> MemorySpaceMap[Index].Length - RegionStart;
> >> }
> >>
> >> + // Always add RO, RP, and XP as all memory is capable of supporting
> these types (they are software
> >> + // constructs, not hardware features) and they are critical to
> maintaining a security boundary
> >> + Capabilities = MemorySpaceMap[Index].Capabilities |
> EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
> >> +
> >> //
> >> - // Set memory attributes according to MTRR attribute and the original
> attribute of descriptor
> >> + // Update GCD capabilities as these may have changed in the page
> table since the GCD was created
> >> + // this follows the same pattern as x86 GCD and Page Table syncing
> >> //
> >> - gDS->SetMemorySpaceAttributes (
> >> - RegionStart,
> >> - RegionLength,
> >> - (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_CACHETYPE_MASK) |
> (MemorySpaceMap[Index].Capabilities & Attributes)
> >> - );
> >> + Status = gDS->SetMemorySpaceCapabilities (
> >> + RegionStart,
> >> + RegionLength,
> >> + Capabilities
> >> + );
> >> +
> >> + if (EFI_ERROR (Status)) {
> >> + DEBUG ((
> >> + DEBUG_ERROR,
> >> + "%a - failed to update GCD capabilities: 0x%llx on memory region:
> 0x%llx length: 0x%llx Status: %r\n",
> >> + __func__,
> >> + Capabilities,
> >> + RegionStart,
> >> + RegionLength,
> >> + Status
> >> + ));
> >> + ASSERT_EFI_ERROR (Status);
> >> + continue;
> >> + }
> >> +
> >> + //
> >> + // Set memory attributes according to the page table attribute and the
> original attribute of descriptor
> >> + //
> >> + Status = gDS->SetMemorySpaceAttributes (
> >> + RegionStart,
> >> + RegionLength,
> >> + (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_CACHETYPE_MASK) | (Attributes & Capabilities)
> >> + );
> >> +
> >> + if (EFI_ERROR (Status)) {
> >> + DEBUG ((
> >> + DEBUG_ERROR,
> >> + "%a - failed to update GCD attributes: 0x%llx on memory region:
> 0x%llx length: 0x%llx Status: %r\n",
> >> + __func__,
> >> + Attributes,
> >> + RegionStart,
> >> + RegionLength,
> >> + Status
> >> + ));
> >> + ASSERT_EFI_ERROR (Status);
> >> + continue;
> >> + }
> >> }
> >>
> >> return EFI_SUCCESS;
> >> --
> >> 2.40.0
> >>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103819): https://edk2.groups.io/g/devel/message/103819
Mute This Topic: https://groups.io/mt/98505340/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list