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.5036.1688760791704083806 for ; Fri, 07 Jul 2023 13:13:11 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=G5pUve5K; 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 2600D20C08FA; Fri, 7 Jul 2023 13:13:11 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 2600D20C08FA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1688760791; bh=wc5JeqicftwrpiZvJgmpx3XHEfgrf5LozlS3NbJTVaE=; h=Date:Subject:From:To:Cc:Reply-To:References:In-Reply-To:From; b=G5pUve5KCJrmqcKoO8uI5LtyYVqYcC9vmbbsqpJZZFcHCG7zj4diTpCqOpnE65nTN s5d9MMaP24B6/COTfgQgUk+0v3yNdg+mokORYw4U/mvu3LnCUDnxq4PAGUTHop7YjT oDdYXiMycLGnio/vmCuylia+dMWXS6eYT4Znobec= Message-ID: Date: Fri, 7 Jul 2023 13:13:10 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.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, ardb@kernel.org 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> <176672827230FBF7.32008@groups.io> In-Reply-To: <176672827230FBF7.32008@groups.io> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 6/7/2023 11:03 AM, Oliver Smith-Denny wrote: > On 6/7/2023 10:31 AM, Ard Biesheuvel wrote: >> On Wed, 7 Jun 2023 at 18:10, Oliver Smith-Denny >> 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=3D753 >>> >>> 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 >> >> =C2=A0=C2=A0 ResourceAttributes =3D ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EF= I_RESOURCE_ATTRIBUTE_PRESENT | >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EF= I_RESOURCE_ATTRIBUTE_INITIALIZED | >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EF= I_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EF= I_RESOURCE_ATTRIBUTE_TESTED >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >> >> 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. >> >=20 > 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 :). >=20 > 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. >=20 > 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. >=20 Hi Ard, I'm returning to this after a couple of weeks of vacation, hopefully with fresh eyes :). The path you describe of resource descriptor HOBs including the RO/RP/XP capabilities does looks like it would work and I do like that it moves these capabilities earlier. That being said, the downside of this approach to me is that it has the core code rely on the platform doing the right thing, which is far from guaranteed. I would prefer to have the core code control the memory protections as much as possible with platform configurability. Platforms have already had the opportunity to set these capabilities in resource descriptor HOBs, but are not. This leads into another related concept, what is the default state of free memory (is it XP, RO, RP, none of them, some combination, etc.). This is also something that should be enforced at the core level, I believe. I will explore this further in a different patch set. For this patchset, I think the right approach would be to set the RP/RO/XP capabilities by default when constructing the GCD. This would also allow the Intel side to not have the workaround they have, where they apply these capabilities when syncing the page table and the GCD after CpuDxe comes up (as I did in this patchset for ARM64). I can spin off a new version with that once I do so some testing. Thanks, Oliver >=20 >> >> >>> >>> 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:=20 >>>> https://github.com/os-d/edk2/tree/osde/sync_aarch64_gcd_capabilities_v= 1 >>>> >>>> >>>> >>>> Cc: Leif Lindholm >>>> >>>> Cc: Ard Biesheuvel >>>> >>>> Cc: Sami Mujawar >>>> >>>> Cc: Michael Kubacki >>>> >>>> Cc: Sean Brogan >>>> >>>> >>>> >>>> Signed-off-by: Oliver Smith-Denny >>>> >>>> --- >>>> >>>> =C2=A0=C2=A0 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 55 +++++++++++++++= ++--- >>>> >>>> =C2=A0=C2=A0 1 file changed, 49 insertions(+), 6 deletions(-) >>>> >>>> >>>> >>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c=20 >>>> 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 ( >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EndIndex; >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 EFI_PHYSICAL_ADDRESS=C2=A0 RegionStart; >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLength; >>>> >>>> +=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Capabilities; >>>> >>>> >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 DEBUG (( >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG_GCD, >>>> >>>> @@ -146,14 +147,56 @@ SetGcdMemorySpaceAttributes ( >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLength =3D Memo= rySpaceMap[Index].BaseAddress +=20 >>>> MemorySpaceMap[Index].Length - RegionStart; >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>> >>>> >>>> >>>> +=C2=A0=C2=A0=C2=A0 // Always add RO, RP, and XP as all memory is capa= ble of=20 >>>> supporting these types (they are software >>>> >>>> +=C2=A0=C2=A0=C2=A0 // constructs, not hardware features) and they are= critical to=20 >>>> maintaining a security boundary >>>> >>>> +=C2=A0=C2=A0=C2=A0 Capabilities =3D MemorySpaceMap[Index].Capabilitie= s |=20 >>>> EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP; >>>> >>>> + >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>>> >>>> -=C2=A0=C2=A0=C2=A0 // Set memory attributes according to MTRR attribu= te and the=20 >>>> original attribute of descriptor >>>> >>>> +=C2=A0=C2=A0=C2=A0 // Update GCD capabilities as these may have chang= ed in the=20 >>>> page table since the GCD was created >>>> >>>> +=C2=A0=C2=A0=C2=A0 // this follows the same pattern as x86 GCD and Pa= ge Table syncing >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>>> >>>> -=C2=A0=C2=A0=C2=A0 gDS->SetMemorySpaceAttributes ( >>>> >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionSt= art, >>>> >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLe= ngth, >>>> >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (MemoryS= paceMap[Index].Attributes &=20 >>>> ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities &=20 >>>> Attributes) >>>> >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >>>> >>>> +=C2=A0=C2=A0=C2=A0 Status =3D gDS->SetMemorySpaceCapabilities ( >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionStart, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLength, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Capabilities >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >>>> >>>> + >>>> >>>> +=C2=A0=C2=A0=C2=A0 if (EFI_ERROR (Status)) { >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG (( >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG_ERROR, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "%a - failed to update GCD= capabilities: 0x%llx on memory=20 >>>> region: 0x%llx length: 0x%llx Status: %r\n", >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __func__, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Capabilities, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionStart, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLength, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 )); >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASSERT_EFI_ERROR (Status); >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; >>>> >>>> +=C2=A0=C2=A0=C2=A0 } >>>> >>>> + >>>> >>>> +=C2=A0=C2=A0=C2=A0 // >>>> >>>> +=C2=A0=C2=A0=C2=A0 // Set memory attributes according to the page tab= le attribute=20 >>>> and the original attribute of descriptor >>>> >>>> +=C2=A0=C2=A0=C2=A0 // >>>> >>>> +=C2=A0=C2=A0=C2=A0 Status =3D gDS->SetMemorySpaceAttributes ( >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionStart, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLength, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (MemorySpaceMap[Index].Attrib= utes &=20 >>>> ~EFI_MEMORY_CACHETYPE_MASK) | (Attributes & Capabilities) >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >>>> >>>> + >>>> >>>> +=C2=A0=C2=A0=C2=A0 if (EFI_ERROR (Status)) { >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG (( >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG_ERROR, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "%a - failed to update GCD= attributes: 0x%llx on memory=20 >>>> region: 0x%llx length: 0x%llx Status: %r\n", >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __func__, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Attributes, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionStart, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RegionLength, >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 )); >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASSERT_EFI_ERROR (Status); >>>> >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 continue; >>>> >>>> +=C2=A0=C2=A0=C2=A0 } >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 } >>>> >>>> >>>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0 return EFI_SUCCESS; >>>> >> >> >> >> >=20 >=20 >=20 >=20