From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.100337.1682960627210258598 for ; Mon, 01 May 2023 10:03:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=gFPOwD1e; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 55CF121C3A90; Mon, 1 May 2023 10:03:46 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 55CF121C3A90 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1682960626; bh=LlS5TYofeGFNo+2XDNTHRYD8pAsYpHUxb0GUkikFJNw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=gFPOwD1ev6GIVwTzlb1qI0xwRvdtBFG+K8NXKbmtzOy2muxjnR0iwhyDj57rUbP+H TSKphZJrrReojq1EGjZhLWXWVbf2D7QOf1vZi5wRVTWgyl+lYFNYgV64qxxtBdHS2r AQfStmaS+vPK7uxoJFm+Vp7E64x6fS1W2TJQAGkI= Message-ID: <04e4ae2c-2e2b-b8b1-b8c9-fe069cbb7ae4@linux.microsoft.com> Date: Mon, 1 May 2023 10:03:46 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes To: Ard Biesheuvel Cc: devel@edk2.groups.io, Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Michael Kubacki , Sean Brogan References: <20230426000930.5748-1-osde@linux.microsoft.com> From: "Oliver Smith-Denny" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 5/1/2023 6:02 AM, Ard Biesheuvel wrote: > On Wed, 26 Apr 2023 at 02:09, 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. >> > > 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/56e9828380b7425678a080bd3a08e7c741af67ba/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 >> Cc: Ard Biesheuvel >> Cc: Sami Mujawar >> Cc: Michael Kubacki >> Cc: Sean Brogan >> >> Signed-off-by: Oliver Smith-Denny >> --- >> 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 >>