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.web11.11076.1675331040856141731 for ; Thu, 02 Feb 2023 01:44:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=oXnx/TVB; 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 390AA61A5A for ; Thu, 2 Feb 2023 09:44:00 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1B9AC4339B for ; Thu, 2 Feb 2023 09:43:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675331039; bh=rVRqGFccLm+EaJ8aRKfuakcIms96rJ3AVurF+SnQ1io=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=oXnx/TVBe+3t1zyT4b8FJhb213b1s3vozka7CBMZFk3Te8XsXhfGDrJsFqWDLUUVO NPiAQ3wTv+AOfF54HFg9d4enxY4ZYiHGtQNQXTNTbUqtwRTNU2fGrKxqpSZkqMCDFt sP3iLXbw4akqo9F3ilojTPK8G4wCcFBsDdXlhbx/vFUAdJT/Y9noFwyB8hTTNIblly /ZadWZeYETq6iYvltwnyBfZUtkxS2YNdK1XkZER24BcgVXTJPG0Ix0GCllxrDC5l3S dqsTiIJqa8lHUpXaI0cVxUf2mEk0mAC5dNVJFIppFJ9R0hCJIhnobYUpK6iilKZs+p PPJ/NbrCMHd+Q== Received: by mail-lj1-f176.google.com with SMTP id bf19so1275021ljb.6 for ; Thu, 02 Feb 2023 01:43:59 -0800 (PST) X-Gm-Message-State: AO0yUKUXKPiBTSP20LM3mJPNWUBhvOl6v+YMXoheqrGqVaLcHgg1ozhU YZ0ADL+WjhDrCohw3tqqY/ecTS3MyFuKQIHG7Zk= X-Google-Smtp-Source: AK7set/F5yzvGFMkqGnzxwpq+tf2aUkcfufhLf4Q/qHf11gaZ3+TvDxvf9BJN1MlXS30NTdlOUuXUVeNcz9sy0rlXH4= X-Received: by 2002:a2e:311:0:b0:290:5b9d:e97 with SMTP id 17-20020a2e0311000000b002905b9d0e97mr790651ljd.187.1675331037623; Thu, 02 Feb 2023 01:43:57 -0800 (PST) MIME-Version: 1.0 References: <20230131223550.1775834-1-ardb@kernel.org> <20230131223550.1775834-5-ardb@kernel.org> <6d3ca765-a196-a2ca-8b42-21a9bf019ae6@taylorbeebe.com> In-Reply-To: <6d3ca765-a196-a2ca-8b42-21a9bf019ae6@taylorbeebe.com> From: "Ard Biesheuvel" Date: Thu, 2 Feb 2023 10:43:46 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol To: devel@edk2.groups.io, t@taylorbeebe.com Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar Content-Type: text/plain; charset="UTF-8" On Wed, 1 Feb 2023 at 19:41, Taylor Beebe wrote: > > Hey Ard, > > Have you encountered complications which stem from the lack of > pre-allocated page table memory on ARM devices utilizing the memory > protection policy? > Interesting. No I haven't, but I agree it is a potential concern. > My observation is the call stack can end up something like: > > 1. MemoryAttributeProtocol->SetMemoryAttributes(EFI_MEMORY_RO) > 2. ArmSetMemoryRegionReadOnly () > 3. SetMemoryRegionAttribute() > 4. UpdateRegionMapping() > 5. UpdateRegionMappingRecursive() > 6. AllocatePages() -> Need memory for a translation table entry > 7. CoreAllocatePages() > 8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN > 9. gCpu->SetMemoryAttributes() > 10. Back to 3 and loop infinitely > So one thing that makes this scenario unlikely (I think) is that by default, all unallocated space has the XP attribute set already, and so allocating a page table with XP attributes should not result in the need for a block entry split, and therefore no allocation for a next level table. Perhaps we need some asserts to diagnose this at runtime? > On 1/31/2023 2:35 PM, Ard Biesheuvel wrote: > > Expose the protocol introduced in v2.10 that permits the caller to > > manage mapping permissions in the page tables. > > > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 + > > ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 + > > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 + > > ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 242 ++++++++++++++++++++ > > 4 files changed, 249 insertions(+) > > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > index e6742f0a25fc..d04958e79e52 100644 > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > @@ -244,6 +244,8 @@ CpuDxeInitialize ( > > &mCpuHandle, > > > > &gEfiCpuArchProtocolGuid, > > > > &mCpu, > > > > + &gEfiMemoryAttributeProtocolGuid, > > > > + &mMemoryAttribute, > > > > NULL > > > > ); > > > > > > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h > > index 8933fa90c4ed..f45d2bc101a3 100644 > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h > > @@ -30,9 +30,12 @@ > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > extern BOOLEAN mIsFlushingGCD; > > > > > > > > +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute; > > > > + > > > > /** > > > > This function registers and enables the handler specified by InterruptHandler for a processor > > > > interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the > > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > index 10792b393fc8..e732e21cb94a 100644 > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > @@ -23,6 +23,7 @@ [Sources.Common] > > CpuDxe.h > > > > CpuMmuCommon.c > > > > Exception.c > > > > + MemoryAttribute.c > > > > > > > > [Sources.ARM] > > > > Arm/Mmu.c > > > > @@ -53,6 +54,7 @@ [LibraryClasses] > > > > > > [Protocols] > > > > gEfiCpuArchProtocolGuid > > > > + gEfiMemoryAttributeProtocolGuid > > > > > > > > [Guids] > > > > gEfiDebugImageInfoTableGuid > > > > diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c > > new file mode 100644 > > index 000000000000..9aeb83fff1b9 > > --- /dev/null > > +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c > > @@ -0,0 +1,242 @@ > > +/** @file > > > > + > > > > + Copyright (c) 2023, Google LLC. All rights reserved. > > > > + > > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > > + > > > > +**/ > > > > + > > > > +#include "CpuDxe.h" > > > > + > > > > +/** > > > > + This function retrieves the attributes of the memory region specified by > > > > + BaseAddress and Length. If different attributes are got from different part > > > > + of the memory region, EFI_NO_MAPPING will be returned. > > > > + > > > > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance. > > > > + @param BaseAddress The physical address that is the start address of > > > > + a memory region. > > > > + @param Length The size in bytes of the memory region. > > > > + @param Attributes Pointer to attributes returned. > > > > + > > > > + @retval EFI_SUCCESS The attributes got for the memory region. > > > > + @retval EFI_INVALID_PARAMETER Length is zero. > > > > + Attributes is NULL. > > > > + @retval EFI_NO_MAPPING Attributes are not consistent cross the memory > > > > + region. > > > > + @retval EFI_UNSUPPORTED The processor does not support one or more > > > > + bytes of the memory resource range specified > > > > + by BaseAddress and Length. > > > > + > > > > +**/ > > > > +STATIC > > > > +EFI_STATUS > > > > +GetMemoryAttributes ( > > > > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This, > > > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > > > + IN UINT64 Length, > > > > + OUT UINT64 *Attributes > > > > + ) > > > > +{ > > > > + UINTN RegionAddress; > > > > + UINTN RegionLength; > > > > + UINTN RegionAttributes; > > > > + UINTN Union; > > > > + UINTN Intersection; > > > > + EFI_STATUS Status; > > > > + > > > > + if ((Length == 0) || (Attributes == NULL)) { > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + > > > > + DEBUG ((DEBUG_INFO, > > > > + "%a: BaseAddress == 0x%lx, Length == 0x%lx\n", > > > > + __FUNCTION__, > > > > + (UINTN)BaseAddress, > > > > + (UINTN)Length > > > > + )); > > > > + > > > > + Union = 0; > > > > + Intersection = MAX_UINTN; > > > > + > > > > + for (RegionAddress = (UINTN)BaseAddress; > > > > + RegionAddress < (UINTN)(BaseAddress + Length); > > > > + RegionAddress += RegionLength) { > > > > + > > > > + Status = GetMemoryRegion (&RegionAddress, > > > > + &RegionLength, > > > > + &RegionAttributes > > > > + ); > > > > + > > > > + DEBUG ((DEBUG_INFO, > > > > + "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n", > > > > + __FUNCTION__, > > > > + RegionAddress, > > > > + RegionLength, > > > > + RegionAttributes > > > > + )); > > > > + > > > > + if (EFI_ERROR (Status)) { > > > > + return EFI_NO_MAPPING; > > > > + } > > > > + > > > > + Union |= RegionAttributes; > > > > + Intersection &= RegionAttributes; > > > > + } > > > > + > > > > + DEBUG ((DEBUG_INFO, > > > > + "%a: Union == %lx, Intersection == %lx\n", > > > > + __FUNCTION__, > > > > + Union, > > > > + Intersection > > > > + )); > > > > + > > > > + if (Union != Intersection) { > > > > + return EFI_NO_MAPPING; > > > > + } > > > > + > > > > + *Attributes = PageAttributeToGcdAttribute (Union) & (EFI_MEMORY_RO | EFI_MEMORY_XP); > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +/** > > > > + This function set given attributes of the memory region specified by > > > > + BaseAddress and Length. > > > > + > > > > + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO. > > > > + > > > > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance. > > > > + @param BaseAddress The physical address that is the start address of > > > > + a memory region. > > > > + @param Length The size in bytes of the memory region. > > > > + @param Attributes The bit mask of attributes to set for the memory > > > > + region. > > > > + > > > > + @retval EFI_SUCCESS The attributes were set for the memory region. > > > > + @retval EFI_INVALID_PARAMETER Length is zero. > > > > + Attributes specified an illegal combination of > > > > + attributes that cannot be set together. > > > > + @retval EFI_UNSUPPORTED The processor does not support one or more > > > > + bytes of the memory resource range specified > > > > + by BaseAddress and Length. > > > > + The bit mask of attributes is not supported for > > > > + the memory resource range specified by > > > > + BaseAddress and Length. > > > > + > > > > +**/ > > > > +STATIC > > > > +EFI_STATUS > > > > +SetMemoryAttributes ( > > > > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This, > > > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > > > + IN UINT64 Length, > > > > + IN UINT64 Attributes > > > > + ) > > > > +{ > > > > + EFI_STATUS Status; > > > > + > > > > + DEBUG ((DEBUG_INFO, > > > > + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n", > > > > + __FUNCTION__, > > > > + (UINTN)BaseAddress, > > > > + (UINTN)Length, > > > > + (UINTN)Attributes > > > > + )); > > > > + > > > > + if ((Length == 0) || > > > > + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) { > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + > > > > + if ((Attributes & EFI_MEMORY_RP) != 0) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + if ((Attributes & EFI_MEMORY_RO) != 0) { > > > > + Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length); > > > > + if (EFI_ERROR (Status)) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + } > > > > + > > > > + if ((Attributes & EFI_MEMORY_XP) != 0) { > > > > + Status = ArmSetMemoryRegionNoExec (BaseAddress, Length); > > > > + if (EFI_ERROR (Status)) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +/** > > > > + This function clears given attributes of the memory region specified by > > > > + BaseAddress and Length. > > > > + > > > > + The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO. > > > > + > > > > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance. > > > > + @param BaseAddress The physical address that is the start address of > > > > + a memory region. > > > > + @param Length The size in bytes of the memory region. > > > > + @param Attributes The bit mask of attributes to clear for the memory > > > > + region. > > > > + > > > > + @retval EFI_SUCCESS The attributes were cleared for the memory region. > > > > + @retval EFI_INVALID_PARAMETER Length is zero. > > > > + Attributes specified an illegal combination of > > > > + attributes that cannot be cleared together. > > > > + @retval EFI_UNSUPPORTED The processor does not support one or more > > > > + bytes of the memory resource range specified > > > > + by BaseAddress and Length. > > > > + The bit mask of attributes is not supported for > > > > + the memory resource range specified by > > > > + BaseAddress and Length. > > > > + > > > > +**/ > > > > +STATIC > > > > +EFI_STATUS > > > > +ClearMemoryAttributes ( > > > > + IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This, > > > > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > > > > + IN UINT64 Length, > > > > + IN UINT64 Attributes > > > > + ) > > > > +{ > > > > + EFI_STATUS Status; > > > > + > > > > + DEBUG ((DEBUG_INFO, > > > > + "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n", > > > > + __FUNCTION__, > > > > + (UINTN)BaseAddress, > > > > + (UINTN)Length, > > > > + (UINTN)Attributes > > > > + )); > > > > + > > > > + if ((Length == 0) || > > > > + ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) { > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + > > > > + if ((Attributes & EFI_MEMORY_RO) != 0) { > > > > + Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length); > > > > + if (EFI_ERROR (Status)) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + } > > > > + > > > > + if ((Attributes & EFI_MEMORY_XP) != 0) { > > > > + Status = ArmClearMemoryRegionNoExec (BaseAddress, Length); > > > > + if (EFI_ERROR (Status)) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = { > > > > + GetMemoryAttributes, > > > > + SetMemoryAttributes, > > > > + ClearMemoryAttributes > > > > +}; > > > > -- > Taylor Beebe > Software Engineer @ Microsoft > > > > >