On 6/7/2023 10:31 AM, Ard Biesheuvel wrote:
On Wed, 7 Jun 2023 at 18:10, Oliver Smith-Denny
<o...@linux.microsoft.com> wrote:
Per the discussion in the memory protections design meeting
this morning, I am kicking this patch back to the top of
the inbox for review. If folks would like me to resend this
patchset since the thread got bogged down with scheduling
meetings, just let me know.
I'll also pull up the BZ link for when the equivalent
change went into the x86 CpuDxe driver in 2017:
https://bugzilla.tianocore.org/show_bug.cgi?id=753
This contains lots of information about why the change went
in on the x86 side (some dead mail links, but they can be
retrieved through some digging). AFAICT, this change wasn't
applied to ARM at the time due to an oversight, not a general
design decision.
Thanks for the background, this is useful.
So I agree that for all system memory regions, we should be setting
the RP, RO and XP capabilities. But what I don't understand is why
these are not set to begin with.
IOW, the resource descriptor HOBs that the initial regions are based
on should have these capabilities set already, and then, we wouldn't
have to do anything to at this point. If there is anything missing
from the generic plumbing to make sure this transformation happens
correctly, we should fix that first, and fix the existing ARM
platforms to set the correct resource attributes.
For example, ArmVirtQemu uses
ResourceAttributes = (
EFI_RESOURCE_ATTRIBUTE_PRESENT |
EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
EFI_RESOURCE_ATTRIBUTE_TESTED
);
for the resource descriptor HOBs, and afaict, this should include
EFI_RESOURCE_ATTRIBUTE_WRITE_PROTECTABLE
EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE
EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE
to accurately describe the region's capabilities.
WIth that out of the way, I wonder if we still need this patch at all.
Thanks,
Ard.
I definitely agree this should be set at GCD initialization. Looking
at the code path you present, I think this could work. My initial
concern was that the resource descriptor HOB was passing attributes
not capabilities, but I see that it actually passes both in a field
called attributes :).
On ARM, as you point out, this looks like we would intercept at
MemoryInitPeiLib when it builds the HOBs. This would be a separate
method from what x86 does, but I can take a look at aligning x86
to initializing the capabilities through the resource descriptor
HOBs.
I'll take a crack at this and see how it shapes up. Seems reasonable
to me, though. I'll need to look into adding memory ranges, we would
want new ranges to have the capabilities, too.
Thanks,
Oliver
On 4/25/2023 5:09 PM, Oliver Smith-Denny 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.
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_llind...@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
Cc: Sami Mujawar <sami.muja...@arm.com>
Cc: Michael Kubacki <mikub...@linux.microsoft.com>
Cc: Sean Brogan <sean.bro...@microsoft.com>
Signed-off-by: Oliver Smith-Denny <o...@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;
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105880): https://edk2.groups.io/g/devel/message/105880
Mute This Topic: https://groups.io/mt/99388013/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-