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.web11.96227.1682467773089618310 for ; Tue, 25 Apr 2023 17:09:33 -0700 Authentication-Results: mx.groups.io; dkim=fail, err=malformed MIME header line: Subject: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from OSD-Desktop.redmond.corp.microsoft.com (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 8184821C2B30; Tue, 25 Apr 2023 17:09:32 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 8184821C2B30 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1682467772; bh=yqvXSX4jhpJgSd6Y3J/1erqruKE53MCIR35RMzLeccM=; h=From:To:Cc:Subject:Date:From; b=G4SHzWrFPdaxU6JYpzACZCyEWFsBEWOz/ZudYlxq1kjWII1qfAPXt/5seTFn+jozO ELiW6uzo+bKIDT/G92f+0TuQejUe+O3s437z6IuZ6PArhu/tt+k3kRI23Txb1XM2om c/4B6C2hk0aQGCc7koGtTbtWs12GJlvc4+81D8z4= From: "Oliver Smith-Denny" To: devel@edk2.groups.io Cc: Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Michael Kubacki , Sean Brogan Subject: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes Date: Tue, 25 Apr 2023 17:09:30 -0700 Message-Id: <20230426000930.5748-1-osde@linux.microsoft.com> X-Mailer: git-send-email 2.40.0 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable When ArmPkg's CpuDxe driver initializes, it attempts to sync the=0D GCD with the page table. However, unlike when the UefiCpuPkg's=0D CpuDxe initializes, the Arm version does not update the GCD=0D capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set=0D the capabilities to be the existing page table attributes for=0D this range, but the UefiCpuPkg CpuDxe sets the above attributes=0D as they are software constructs, possible to set for any memory=0D hardware).=0D =0D As a result, when the GCD attributes are attempted to be set=0D against the old GCD capabilities, attributes that are set in the=0D page table can get lost because the new attributes are not in the=0D old GCD capabilities (and yet they are already set in the page=0D table) meaning that the attempted sync between the GCD and the=0D page table was a failure and drivers querying one vs the other=0D will see different state. This can lead to RWX memory regions=0D even with the no-execute policy set, because core drivers (such=0D as NonDiscoverablePciDeviceIo, to be fixed up in a later patchset)=0D allocate pages, query the GCD attributes, attempt to set a new=0D cache attribute and end up clearing the XP bit in the page table=0D because the GCD attributes do not have XP set.=0D =0D This patch follows the UefiCpuPkg pattern and adds=0D EFI_MEMORY_[RO|RP|XP] to the GCD capabilities during CpuDxe=0D initialization. This ensures that memory regions which already have=0D these attributes set get them set in the GCD attributes, properly=0D syncing between the GCD and the page table.=0D =0D This mitigates the issue seen, however, additional investigations=0D into setting the GCD attributes earlier and maintaining a better=0D sync between the GCD and the page table are being done.=0D =0D Feedback on this proposal is greatly appreciated, particularly=0D any pitfalls or more architectural solutions to issues seen=0D with syncing the GCD and the page table.=0D =0D PR: https://github.com/tianocore/edk2/pull/4311=0D Personal branch: https://github.com/os-d/edk2/tree/osde/sync_aarch64_gcd_ca= pabilities_v1=0D =0D Cc: Leif Lindholm =0D Cc: Ard Biesheuvel =0D Cc: Sami Mujawar =0D Cc: Michael Kubacki =0D Cc: Sean Brogan =0D =0D Signed-off-by: Oliver Smith-Denny =0D ---=0D ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 55 +++++++++++++++++---=0D 1 file changed, 49 insertions(+), 6 deletions(-)=0D =0D diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/C= puMmuCommon.c=0D index 2e73719dce04..3ef0380e084f 100644=0D --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c=0D +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c=0D @@ -90,6 +90,7 @@ SetGcdMemorySpaceAttributes (=0D UINTN EndIndex;=0D EFI_PHYSICAL_ADDRESS RegionStart;=0D UINT64 RegionLength;=0D + UINT64 Capabilities;=0D =0D DEBUG ((=0D DEBUG_GCD,=0D @@ -146,14 +147,56 @@ SetGcdMemorySpaceAttributes (=0D RegionLength =3D MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[= Index].Length - RegionStart;=0D }=0D =0D + // Always add RO, RP, and XP as all memory is capable of supporting th= ese types (they are software=0D + // constructs, not hardware features) and they are critical to maintai= ning a security boundary=0D + Capabilities =3D MemorySpaceMap[Index].Capabilities | EFI_MEMORY_RO | = EFI_MEMORY_RP | EFI_MEMORY_XP;=0D +=0D //=0D - // Set memory attributes according to MTRR attribute and the original = attribute of descriptor=0D + // Update GCD capabilities as these may have changed in the page table= since the GCD was created=0D + // this follows the same pattern as x86 GCD and Page Table syncing=0D //=0D - gDS->SetMemorySpaceAttributes (=0D - RegionStart,=0D - RegionLength,=0D - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK)= | (MemorySpaceMap[Index].Capabilities & Attributes)=0D - );=0D + Status =3D gDS->SetMemorySpaceCapabilities (=0D + RegionStart,=0D + RegionLength,=0D + Capabilities=0D + );=0D +=0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((=0D + DEBUG_ERROR,=0D + "%a - failed to update GCD capabilities: 0x%llx on memory region: = 0x%llx length: 0x%llx Status: %r\n",=0D + __func__,=0D + Capabilities,=0D + RegionStart,=0D + RegionLength,=0D + Status=0D + ));=0D + ASSERT_EFI_ERROR (Status);=0D + continue;=0D + }=0D +=0D + //=0D + // Set memory attributes according to the page table attribute and the= original attribute of descriptor=0D + //=0D + Status =3D gDS->SetMemorySpaceAttributes (=0D + RegionStart,=0D + RegionLength,=0D + (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHET= YPE_MASK) | (Attributes & Capabilities)=0D + );=0D +=0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((=0D + DEBUG_ERROR,=0D + "%a - failed to update GCD attributes: 0x%llx on memory region: 0x= %llx length: 0x%llx Status: %r\n",=0D + __func__,=0D + Attributes,=0D + RegionStart,=0D + RegionLength,=0D + Status=0D + ));=0D + ASSERT_EFI_ERROR (Status);=0D + continue;=0D + }=0D }=0D =0D return EFI_SUCCESS;=0D -- =0D 2.40.0=0D =0D