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.2252.1686154202261750363 for ; Wed, 07 Jun 2023 09:10:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=GwHWDCe4; 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 AA45B20BE4BA; Wed, 7 Jun 2023 09:10:01 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AA45B20BE4BA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686154201; bh=mc9vjYcdCacLT83JRKSTQRCugkFVsOYASwHeGpYYvLc=; h=Date:Subject:From:To:Cc:Reply-To:References:In-Reply-To:From; b=GwHWDCe4OMQP4XMCPP8oru531D7aKEiqvJgDAn6Q8CclOUnb8dJkQZPygzrcTmWMb VLh+m1Tbgm3xS0MeLMDq8tLJb8BzzPFpJMHGGIyoaM49Q1tPu5wU3iySje8I/vmCEC dwLKHKZH45o/QNyaGKRkH9g24j3jpua8mn5sz9L4= Message-ID: Date: Wed, 7 Jun 2023 09:10:01 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes From: "Oliver Smith-Denny" To: devel@edk2.groups.io Cc: Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Michael Kubacki , Sean Brogan Reply-To: devel@edk2.groups.io, osde@linux.microsoft.com References: <1759538694580A69.7408@groups.io> In-Reply-To: <1759538694580A69.7408@groups.io> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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, 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 > > 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; >