From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f47.google.com (mail-pj1-f47.google.com [209.85.216.47]) by mx.groups.io with SMTP id smtpd.web11.32404.1675276864390865658 for ; Wed, 01 Feb 2023 10:41:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@taylorbeebe.com header.s=google header.b=UWpPdBxa; spf=pass (domain: taylorbeebe.com, ip: 209.85.216.47, mailfrom: t@taylorbeebe.com) Received: by mail-pj1-f47.google.com with SMTP id e8-20020a17090a9a8800b0022c387f0f93so3137217pjp.3 for ; Wed, 01 Feb 2023 10:41:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=taylorbeebe.com; s=google; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=s2OdVMCzUSwnxXB88tP8a10E8u8jiwqv5MJ3nqD2A3c=; b=UWpPdBxar4suhtRwYEpU1uPEMhB1udjX+/8ficCPrr4Z64FkkUpzYAXbXohH5CC7u2 BhzCjhe7A7nfeZqUwBf3jxyqff0CZcEY8xQ0ueM5Br/3+ZskDmx7cp0tcFPMNrhyoKfc V3e+bgJIU+Jehs4RrAIYOsgirGoKZsdfhDYglRlpOmIIzaArOLQV+lA9kaDqSllz++KC NnUIsfHB9j/6qDchq7cDHHmei1bSqnq/9NksBIecoKP0iYiyCfDPwBt3lvBdaQrVE3wb TKlGnsMWtCRhzGhY02lNiS+zVVoR0eOUCHuB+2UfZ5j360tKJXvu0ss9RNPlMA74lHF4 bZFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=s2OdVMCzUSwnxXB88tP8a10E8u8jiwqv5MJ3nqD2A3c=; b=ea3vWYaczPc9vLlQqg870xyaF750o8jTVFJIWWtNH5DG7gKyhRnZIQqPgWJe0V3HBP MisKZvw7PabcnJqYe3HX3hSGTEsvtnQB6cHFff2UcsFuR/HRFsx10sVsNVW3n3Baq7RI TzMt4M6zWEMBt4X4Ha4AlykaEI5bJI1cz8W5JoGRraHd6O3Y4psJRo26votUwtypFBrv julZxOQSXyCf/c/dz/Ca09sl0UOQpN7nUg406mfTPdFE788sbEY+vt+287sjSqI5KUNm RQyvWKv0jPkabUChx0yxdd+MB9GYsVtmvTsg6JVEDpwaIXc83tjK7BkyiSZqJ7jAFKGs eGMQ== X-Gm-Message-State: AO0yUKWEyDZgWmeDvrxIuMXxPkdDCSkfoSCNYD8tdLfI4s7nJPzhb/Rh uVC4h9n4dV+tIBvqOQJBeoH2fibJ7xQQXXzi534= X-Google-Smtp-Source: AK7set/2fRtSO2FOqYdi2TEgjsJgiwT1OuTDH+GIJa/bdsVAREw137lGuClXbIEp02wYeWjuVH/G3w== X-Received: by 2002:a17:902:ce84:b0:196:7128:f803 with SMTP id f4-20020a170902ce8400b001967128f803mr4721616plg.23.1675276863318; Wed, 01 Feb 2023 10:41:03 -0800 (PST) Return-Path: Received: from [192.168.50.162] ([50.46.230.135]) by smtp.gmail.com with ESMTPSA id az9-20020a170902a58900b00196349cf655sm8567180plb.282.2023.02.01.10.41.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 01 Feb 2023 10:41:02 -0800 (PST) Message-ID: <6d3ca765-a196-a2ca-8b42-21a9bf019ae6@taylorbeebe.com> Date: Wed, 1 Feb 2023 10:41:03 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol To: devel@edk2.groups.io, ardb@kernel.org Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar References: <20230131223550.1775834-1-ardb@kernel.org> <20230131223550.1775834-5-ardb@kernel.org> From: "Taylor Beebe" In-Reply-To: <20230131223550.1775834-5-ardb@kernel.org> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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? 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 Thanks! -Taylor 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