From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f176.google.com (mail-pf1-f176.google.com [209.85.210.176]) by mx.groups.io with SMTP id smtpd.web10.20122.1675451340642820480 for ; Fri, 03 Feb 2023 11:09:00 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@taylorbeebe.com header.s=google header.b=Lq4bZOdq; spf=pass (domain: taylorbeebe.com, ip: 209.85.210.176, mailfrom: t@taylorbeebe.com) Received: by mail-pf1-f176.google.com with SMTP id z3so4437601pfb.2 for ; Fri, 03 Feb 2023 11:09:00 -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:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=NgL7BH58A2kFyfZl4oKQTtkNKXeFgeFlgkVsB7lwSoU=; b=Lq4bZOdq3A2e2G/59w06v4QtfcBZDim/kRV+io4hjr09DDp2utg/Odtm3n29efbTML sTpHszO2SYe58wmZeJ80XyKny9yPqedb1zWo03ozDQ5+tteMcUW3AFu9/5Wql4fBMHwr Et+rPrixwBwTgiIy2bnVKzzLkALiAts5HYw9WgsLHUBYaTCKMqy5hTAubs3omxdoJdh/ lHU75eLj6kzfH50TSKS+ByjYJKQDfTU+Pza00HWNFzADP05WgjA7pgrX3GxdJ8NfMk00 N96CYWH8x94uKtBrdVxeG+S7fUH4uomu9E3C0Z8ciJvbxdQN2FyMalJVw0xyi8QLiXNT RO8Q== 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:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=NgL7BH58A2kFyfZl4oKQTtkNKXeFgeFlgkVsB7lwSoU=; b=C/opy2Ak8OojPGyXfW+jrYCavNOlck4dy89Ud7ynl5sohlF64eonQLePMS2xc9vZ51 C0I6jk4q//Rf15lvuvuil2iuEfu7OU673FMvOMpXX4T9DmoPT4D/GUfQkWJGUiSCFvJk fl1NxLlZs9rF99C+xp7gXXdIKQ+UkC4Dmjya2Qm51sm+1KKYuori4KIiMWpKoHKSuq/t a2oKhuIN0HEcYg0dxHNyNKbFTFSf4SK8ZQD15ZzE/nRjttoI0KDKo1AwoR4gnhPzSRhj /C7bU/j/XqaAhUMMCcOfG5mqb5TYV4a73Bf6N2sEYGoiG4yzt+mil4mvg+aM3vnl1MJh FYjQ== X-Gm-Message-State: AO0yUKVJc2NmYnX2n2tUlMYcky3d0GCtbF+I19xxH4nbLFkpHU1ExAjV 6sODX+8+JZGQgqCvVCS/PSdJtw== X-Google-Smtp-Source: AK7set8luEoBvRxN1IGAQQKrFOqwykgknXm2ScJx4I5M8YfYaF0bLWvyGnrmw4WgBKS6mOORs6vOTA== X-Received: by 2002:a62:501:0:b0:594:3fde:40c6 with SMTP id 1-20020a620501000000b005943fde40c6mr4025554pff.25.1675451339773; Fri, 03 Feb 2023 11:08:59 -0800 (PST) Return-Path: Received: from [192.168.50.162] ([50.46.230.135]) by smtp.gmail.com with ESMTPSA id k14-20020aa792ce000000b00580cc63dce8sm2157647pfa.77.2023.02.03.11.08.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 Feb 2023 11:08:59 -0800 (PST) Message-ID: Date: Fri, 3 Feb 2023 11:08:58 -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: Ard Biesheuvel , devel@edk2.groups.io 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> <6d3ca765-a196-a2ca-8b42-21a9bf019ae6@taylorbeebe.com> From: "Taylor Beebe" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2/2/2023 1:43 AM, Ard Biesheuvel wrote: > 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? Ah yes, you're right. On Project Mu, we make an explicit check to the attributes of the region in ApplyMemoryProtectionPolicy() to determine if the attributes need to be updated. We do this to account for the introduction of the memory attribute protocol and to mitigate a rare issue where an allocation might not have consistent attributes when allocating a type other than BootServicesData. This change to ApplyMemoryProtectionPolicy() means an infinite loop is possible during the memory protection initialization phase. > >> 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 >> >> >> >> >> -- Taylor Beebe Software Engineer @ Microsoft