From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.1818.1685719094305701288 for ; Fri, 02 Jun 2023 08:18:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=M5TwYR7e; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B2B5560C6D; Fri, 2 Jun 2023 15:18:13 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7B02BC433D2; Fri, 2 Jun 2023 15:18:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685719093; bh=gsqpn334/xQmvUsTtOKCbf0sussxdQqNqGLagURpFRw=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=M5TwYR7es/+Sh1dgsMR8ET2v4VHIs9YgLythxI4g7BSDD6dN7WTzYi/lCSm/9vD9n laMdRVmr2uJ1cPfT8USqNlTwEIM5eLFOJtT1UYLi3vmvpMsu3DgntENuiZM/0OJFMH BNPnPoapoMuaCMillOZzfuHCxUvru89GCysjq9sSbaRh6yq/L/44+KXv8uLc1KHDAP bgNoZT7RHeqwBBwQh/BXX9IWfAEz5k5R+oGZUsWJoOwKE3fui/4YMP8kGw5r+eYUAw ZdeyH+oR24375ClEN52+ipepyl0b+QJovJbr2DSJI/VJK0EYAxSKr6AiitvLj6LqBi 3Y7tThCxDRN0w== From: "Ard Biesheuvel" To: devel@edk2.groups.io Cc: Ard Biesheuvel , Ray Ni , Jiewen Yao , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , Dandan Bi , Dun Tan , Liming Gao , "Kinney, Michael D" , Leif Lindholm , Michael Kubacki Subject: [PATCH v2 4/7] ArmPkg/ArmMmuLib: Extend API to manage memory permissions better Date: Fri, 2 Jun 2023 17:17:36 +0200 Message-Id: <20230602151739.3600820-5-ardb@kernel.org> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230602151739.3600820-1-ardb@kernel.org> References: <20230602151739.3600820-1-ardb@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Currently, ArmSetMemoryAttributes () takes a combination of EFI_MEMORY_xx constants describing the memory type and permission attributes that should be set on a region of memory. In cases where the memory type is omitted, we assume that the memory permissions being set are final, and that existing memory permissions can be discarded. This is problematic, because we aim to map memory non-executable (EFI_MEMORY_XP) by default, and only relax this requirement for code regions that are mapped read-only (EFI_MEMORY_RO). Currently, setting one permission clears the other, and so code managing these permissions has to be aware of the existing permissions in order to be able to preserve them, and this is not always tractable (e.g., the UEFI memory attribute protocol implements an abstraction that promises to preserve memory permissions that it is not operating on explicitly). So let's add an AttributeMask parameter to ArmSetMemoryAttributes(), which is permitted to be non-zero if no memory type is being provided, in which case only memory permission attributes covered in the mask will be affected by the update. Signed-off-by: Ard Biesheuvel --- ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 2 +- ArmPkg/Include/Library/ArmMmuLib.h | 36 +++++++- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 +++++++++++- ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 88 +++++++++++++++++--- ArmPkg/Library/OpteeLib/Optee.c | 2 +- 5 files changed, 165 insertions(+), 15 deletions(-) diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/C= puMmuCommon.c index 2e73719dce04ceb5..2d60c7d24dc05ee9 100644 --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c @@ -217,7 +217,7 @@ CpuSetMemoryAttributes ( if (EFI_ERROR (Status) || (RegionArmAttributes !=3D ArmAttributes) ||=0D ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))=0D {=0D - return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);=0D + return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);= =0D } else {=0D return EFI_SUCCESS;=0D }=0D diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/Ar= mMmuLib.h index 4cf59a1e376b123c..91d112314fdf4859 100644 --- a/ArmPkg/Include/Library/ArmMmuLib.h +++ b/ArmPkg/Include/Library/ArmMmuLib.h @@ -92,11 +92,45 @@ ArmReplaceLiveTranslationEntry ( IN BOOLEAN DisableMmu=0D );=0D =0D +/**=0D + Set the requested memory permission attributes on a region of memory.=0D +=0D + BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D +=0D + If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB),= the=0D + region is mapped according to this memory type, and additional memory=0D + permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as we= ll,=0D + discarding any permission attributes that are currently set for the regi= on.=0D + AttributeMask is ignored in this case, and must be set to 0x0.=0D +=0D + If Attributes contains only a combination of memory permission attribute= s=0D + (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing= =0D + memory type, even if it is not uniformly set across the region. In this = case,=0D + AttributesMask may be set to a mask of permission attributes, and memory= =0D + permissions omitted from this mask will not be updated for any page in t= he=0D + region. All attributes appearing in Attributes must appear in AttributeM= ask=0D + as well. (Attributes & ~AttributeMask must produce 0x0)=0D +=0D + @param[in] BaseAddress The physical address that is the start addre= ss of=0D + a memory region.=0D + @param[in] Length The size in bytes of the memory region.=0D + @param[in] Attributes Mask of memory attributes to set.=0D + @param[in] AttributeMask Mask of memory attributes to take into accou= nt.=0D +=0D + @retval EFI_SUCCESS The attributes were set for the memory reg= ion.=0D + @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably alig= ned.=0D + Invalid combination of Attributes and=0D + AttributeMask.=0D + @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due= to=0D + lack of system resources.=0D +=0D +**/=0D EFI_STATUS=0D ArmSetMemoryAttributes (=0D IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D IN UINT64 Length,=0D - IN UINT64 Attributes=0D + IN UINT64 Attributes,=0D + IN UINT64 AttributeMask=0D );=0D =0D #endif // ARM_MMU_LIB_H_=0D diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Libr= ary/ArmMmuLib/AArch64/ArmMmuLibCore.c index 7ed758fbbc699732..22623572b9cb931c 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -469,11 +469,45 @@ GcdAttributeToPageAttribute ( return PageAttributes;=0D }=0D =0D +/**=0D + Set the requested memory permission attributes on a region of memory.=0D +=0D + BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D +=0D + If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB),= the=0D + region is mapped according to this memory type, and additional memory=0D + permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as we= ll,=0D + discarding any permission attributes that are currently set for the regi= on.=0D + AttributeMask is ignored in this case, and must be set to 0x0.=0D +=0D + If Attributes contains only a combination of memory permission attribute= s=0D + (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing= =0D + memory type, even if it is not uniformly set across the region. In this = case,=0D + AttributesMask may be set to a mask of permission attributes, and memory= =0D + permissions omitted from this mask will not be updated for any page in t= he=0D + region. All attributes appearing in Attributes must appear in AttributeM= ask=0D + as well. (Attributes & ~AttributeMask must produce 0x0)=0D +=0D + @param[in] BaseAddress The physical address that is the start addre= ss of=0D + a memory region.=0D + @param[in] Length The size in bytes of the memory region.=0D + @param[in] Attributes Mask of memory attributes to set.=0D + @param[in] AttributeMask Mask of memory attributes to take into accou= nt.=0D +=0D + @retval EFI_SUCCESS The attributes were set for the memory reg= ion.=0D + @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably alig= ned.=0D + Invalid combination of Attributes and=0D + AttributeMask.=0D + @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due= to=0D + lack of system resources.=0D +=0D +**/=0D EFI_STATUS=0D ArmSetMemoryAttributes (=0D IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D IN UINT64 Length,=0D - IN UINT64 Attributes=0D + IN UINT64 Attributes,=0D + IN UINT64 AttributeMask=0D )=0D {=0D UINT64 PageAttributes;=0D @@ -490,6 +524,22 @@ ArmSetMemoryAttributes ( PageAttributes &=3D TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;= =0D PageAttributeMask =3D ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |=0D TT_PXN_MASK | TT_XN_MASK | TT_AF);=0D + if (AttributeMask !=3D 0) {=0D + if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMO= RY_XP)) !=3D 0) ||=0D + ((Attributes & ~AttributeMask) !=3D 0))=0D + {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D + // Add attributes omitted from AttributeMask to the set of attribute= s to preserve=0D + PageAttributeMask |=3D GcdAttributeToPageAttribute (~AttributeMask) = &=0D + (TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF= );=0D + }=0D + } else {=0D + ASSERT (AttributeMask =3D=3D 0);=0D + if (AttributeMask !=3D 0) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D }=0D =0D return UpdateRegionMapping (=0D diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Librar= y/ArmMmuLib/Arm/ArmMmuLibUpdate.c index 299d38ad07e85059..61405965a73eaeb8 100644 --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c @@ -10,6 +10,7 @@ #include =0D =0D #include =0D +#include =0D #include =0D #include =0D #include =0D @@ -451,31 +452,96 @@ SetMemoryAttributes ( }=0D =0D /**=0D - Update the permission or memory type attributes on a range of memory.=0D + Set the requested memory permission attributes on a region of memory.=0D =0D - @param BaseAddress The start of the region.=0D - @param Length The size of the region.=0D - @param Attributes A mask of EFI_MEMORY_xx constants.=0D + BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D =0D - @retval EFI_SUCCESS The attributes were set successfully.=0D - @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient m= emory.=0D + If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB),= the=0D + region is mapped according to this memory type, and additional memory=0D + permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as we= ll,=0D + discarding any permission attributes that are currently set for the regi= on.=0D + AttributeMask is ignored in this case, and must be set to 0x0.=0D +=0D + If Attributes contains only a combination of memory permission attribute= s=0D + (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing= =0D + memory type, even if it is not uniformly set across the region. In this = case,=0D + AttributesMask may be set to a mask of permission attributes, and memory= =0D + permissions omitted from this mask will not be updated for any page in t= he=0D + region. All attributes appearing in Attributes must appear in AttributeM= ask=0D + as well. (Attributes & ~AttributeMask must produce 0x0)=0D +=0D + @param[in] BaseAddress The physical address that is the start addre= ss of=0D + a memory region.=0D + @param[in] Length The size in bytes of the memory region.=0D + @param[in] Attributes Mask of memory attributes to set.=0D + @param[in] AttributeMask Mask of memory attributes to take into accou= nt.=0D +=0D + @retval EFI_SUCCESS The attributes were set for the memory reg= ion.=0D + @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably alig= ned.=0D + Invalid combination of Attributes and=0D + AttributeMask.=0D + @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due= to=0D + lack of system resources.=0D =0D **/=0D EFI_STATUS=0D ArmSetMemoryAttributes (=0D IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D IN UINT64 Length,=0D - IN UINT64 Attributes=0D + IN UINT64 Attributes,=0D + IN UINT64 AttributeMask=0D )=0D {=0D + UINT32 TtEntryMask;=0D +=0D + if (((BaseAddress | Length) & EFI_PAGE_MASK) !=3D 0) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D + if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) =3D=3D 0) {=0D + //=0D + // No memory type was set in Attributes, so we are going to update the= =0D + // permissions only.=0D + //=0D + if (AttributeMask !=3D 0) {=0D + if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMO= RY_XP)) !=3D 0) ||=0D + ((Attributes & ~AttributeMask) !=3D 0))=0D + {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D + } else {=0D + AttributeMask =3D EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;=0D + }=0D +=0D + TtEntryMask =3D 0;=0D + if ((AttributeMask & EFI_MEMORY_RP) !=3D 0) {=0D + TtEntryMask |=3D TT_DESCRIPTOR_SECTION_AF;=0D + }=0D +=0D + if ((AttributeMask & EFI_MEMORY_RO) !=3D 0) {=0D + TtEntryMask |=3D TT_DESCRIPTOR_SECTION_AP_MASK;=0D + }=0D +=0D + if ((AttributeMask & EFI_MEMORY_XP) !=3D 0) {=0D + TtEntryMask |=3D TT_DESCRIPTOR_SECTION_XN_MASK;=0D + }=0D + } else {=0D + ASSERT (AttributeMask =3D=3D 0);=0D + if (AttributeMask !=3D 0) {=0D + return EFI_INVALID_PARAMETER;=0D + }=0D +=0D + TtEntryMask =3D TT_DESCRIPTOR_SECTION_TYPE_MASK |=0D + TT_DESCRIPTOR_SECTION_XN_MASK |=0D + TT_DESCRIPTOR_SECTION_AP_MASK |=0D + TT_DESCRIPTOR_SECTION_AF;=0D + }=0D +=0D return SetMemoryAttributes (=0D BaseAddress,=0D Length,=0D Attributes,=0D - TT_DESCRIPTOR_SECTION_TYPE_MASK |=0D - TT_DESCRIPTOR_SECTION_XN_MASK |=0D - TT_DESCRIPTOR_SECTION_AP_MASK |=0D - TT_DESCRIPTOR_SECTION_AF=0D + TtEntryMask=0D );=0D }=0D =0D diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Opte= e.c index 48e33cb3d5ee4ab6..46464f17ef06653e 100644 --- a/ArmPkg/Library/OpteeLib/Optee.c +++ b/ArmPkg/Library/OpteeLib/Optee.c @@ -86,7 +86,7 @@ OpteeSharedMemoryRemap ( return EFI_BUFFER_TOO_SMALL;=0D }=0D =0D - Status =3D ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB)= ;=0D + Status =3D ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB,= 0);=0D if (EFI_ERROR (Status)) {=0D return Status;=0D }=0D --=20 2.39.2