From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 2E0C87803CE for ; Thu, 15 Feb 2024 07:51:11 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=IET8qhV9j5K/LnH+D6I97qvVUvJAlt3q4b06O8oS6oI=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1707983470; v=1; b=LV3rU7eFV+A6ThHy8i+PyXafPEZV9BKi89eWQHAN/hwUSsPLEgJkPhBdU3XmUr+5pnKsy0V9 C5GevtvJ3wk9QYZ24Gq29FOtP3Q85pXx+7JXzC+RVgQzZxaXDBRrhjZcD3CYPkZk4R9ogtyeg6E KJ8B53tS0I281aG42xTwKPSk= X-Received: by 127.0.0.2 with SMTP id vWwHYY7687511xsInOm4XUhn; Wed, 14 Feb 2024 23:51:10 -0800 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.9345.1707983469844614184 for ; Wed, 14 Feb 2024 23:51:09 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 3EB4C61C1E for ; Thu, 15 Feb 2024 07:51:09 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id DC566C433C7 for ; Thu, 15 Feb 2024 07:51:08 +0000 (UTC) X-Received: by mail-lf1-f51.google.com with SMTP id 2adb3069b0e04-5128812662eso364092e87.0 for ; Wed, 14 Feb 2024 23:51:08 -0800 (PST) X-Gm-Message-State: g8qPonNoyemgz6NYCP9tWuigx7686176AA= X-Google-Smtp-Source: AGHT+IF5KS3CayOFQArC1zAoQcz+3qori3k9uoERd95NIR/xYiia5+ptNFUfepXU2KB2SqCmLsIXB+3faNJ9Ny1X0Ds= X-Received: by 2002:a05:6512:535:b0:511:697a:3e71 with SMTP id o21-20020a056512053500b00511697a3e71mr861868lfc.45.1707983466918; Wed, 14 Feb 2024 23:51:06 -0800 (PST) MIME-Version: 1.0 References: <20240215003412.30983-1-osde@linux.microsoft.com> In-Reply-To: <20240215003412.30983-1-osde@linux.microsoft.com> From: "Ard Biesheuvel" Date: Thu, 15 Feb 2024 08:50:55 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations To: devel@edk2.groups.io, osde@linux.microsoft.com Cc: Leif Lindholm , Sami Mujawar , Liming Gao Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=LV3rU7eF; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hey Oliver, On Thu, 15 Feb 2024 at 01:34, Oliver Smith-Denny wrote: > > Currently, there are multiple issues when page or pool guards are > allocated for runtime and ACPI memory regions that are aligned to > non-EFI_PAGE_SIZE alignments. Multiple other issues have been fixed > for these same systems (notably ARM64 which has a 64k runtime page > allocation granularity) recently. The heap guard system is only > built to support 4k guard pages and 4k alignment. > > Today, the address returned to a caller of AllocatePages will not > be aligned correctly to the runtime page allocation granularity, > because the heap guard system does not take non-4k alignment > requirements into consideration. > > However, even with this bug fixed, the Memory Allocation Table > cannot be produced and an OS with a larger than 4k page granularity > will not have aligned memory regions because the guard pages are > reported as part of the same memory allocation. So what would have > been, on an ARM64 system, a 64k runtime memory allocation is actually > a 72k memory allocation as tracked by the Page.c code because the > guard pages are tracked as part of the same allocation. This is a > core function of the current heap guard architecture. > > This could also be fixed with rearchitecting the heap guard system to > respect alignment requirements and shift the guard pages inside of the > outer rounded allocation or by having guard pages be the runtime > granularity. Both of these approaches have issues, in the former, the > allocator of runtime memory would get an address that was not aligned > with the runtime granularity (the head guard would be, but not the > usuable address), which seems fraught with peril. This would be my preference, and I wouldn't expect huge problems with code expecting a certain alignment for such allocations. The 64k requirement is entirely to ensure that the OS does not have to guess how it should map 64k pages that have conflicting memory attributes due to being covered by two different misaligned entries in the UEFI memory map. This is also why this is important for the MAT and runtime services code/data regions: without 64k alignment, there will be a piece in the middle of each runtime DXE that requires both write and execute permissions. > In the latter case, > an immense amount of memory is wasted to support such large guard pages, > and with pool guard many systems could not support an additional 128k > allocation for all runtime memory. > Agreed. > The simpler and safer solution is to disallow page and pool guards for > runtime memory allocations for systems that have a runtime granularity > greater than the EFI_PAGE_SIZE (4k). The usefulness of such guards is > limited, as OSes do not map guard pages today, so there is only boot > time protection of these ranges, which runtime memory is typically > used minimally in boot time. This also prevents other bugs from being > exposed by using guards for regions that have a non-4k alignment > requirement, as again, multiple have cropped up because the heap guard > system was not built to support it. > > This patch adds both a static assert to ensure that either the runtime > granularity is the EFI_PAGE_SIZE or that the PCD bits are not set to > enable heap guard for runtime memory regions. It also adds a check in > the page and pool allocation system to ensure that at runtime we are not > allocating a runtime region and attempt to guard it (the PCDs are close to > being removed in favor of dynamic heap guard configurations). > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4674 > Github PR: https://github.com/tianocore/edk2/pull/5378 > > Cc: Leif Lindholm > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Liming Gao > > Signed-off-by: Oliver Smith-Denny > --- > MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++ > MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 ++++++++++++++ > MdeModulePkg/Core/Dxe/Mem/Page.c | 11 +++++++++++ > MdeModulePkg/Core/Dxe/Mem/Pool.c | 11 +++++++++++ > 4 files changed, 46 insertions(+) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index a2cd83345f5b..884734aff592 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild] > # free pages for all of them. The page allocation for the type related to > # cleared bits keeps the same as ususal. > # > + # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData, > + # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have I looked at the EFI spec again, and EfiACPIReclaimMemory is not actually listed as a memory type that has this 64k alignment requirement. This makes sense, given that this memory type has no significance to the firmware itself, only to the OS. OTOH, reserved memory does appear there. So I suggest we fix that first, and then drop any mention of EfiACPIReclaimMemory from this patch. At least we'll have heap guard coverage for ACPI table allocations on arm64 going forward. The logic in question was added in 2007 in commit 28a00297189c, so this was probably the rule on Itanium, but that support is long gone. diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c index 6497af573353..755b36527d38 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Page.c +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c @@ -1301,7 +1301,7 @@ CoreInternalAllocatePages ( Alignment = DEFAULT_PAGE_ALLOCATION_GRANULARITY; - if ((MemoryType == EfiACPIReclaimMemory) || + if ((MemoryType == EfiReservedMemoryType) || (MemoryType == EfiACPIMemoryNVS) || (MemoryType == EfiRuntimeServicesCode) || (MemoryType == EfiRuntimeServicesData)) (there is another occurrence in MdeModulePkg/Core/Pei/Memory/MemoryServices.c) > + # RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE. This is to preserve alignment requirements > + # without extending the page guard size to very large granularities. > + # > # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask. > # > # Below is bit mask for this PCD: (Order is same as UEFI spec)
> @@ -1058,6 +1063,11 @@ [PcdsFixedAtBuild] > # if there's enough free memory for all of them. The pool allocation for the > # type related to cleared bits keeps the same as ususal. > # > + # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData, > + # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have > + # RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE. This is to preserve alignment requirements > + # without extending the page guard size to very large granularities. > + # > # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask. > # > # Below is bit mask for this PCD: (Order is same as UEFI spec)
> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h > index 24b4206c0e02..d4f283977b04 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h > @@ -469,4 +469,18 @@ PromoteGuardedFreePages ( > > extern BOOLEAN mOnGuarding; > > +// > +// the heap guard system does not support non-EFI_PAGE_SIZE alignments > +// architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY > +// cannot have EfiRuntimeServicesCode, EfiRuntimeServicesData, EfiACPIReclaimMemory, > +// and EfiACPIMemoryNVS guarded. OSes do not map guard pages anyway, so this is a > +// minimal loss. Not guarding prevents alignment mismatches > +// > +STATIC_ASSERT ( > + RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE || > + (((FixedPcdGet64 (PcdHeapGuardPageType) & 0x660) == 0) && > + ((FixedPcdGet64 (PcdHeapGuardPoolType) & 0x660) == 0)), > + "Unsupported Heap Guard configuration on system with greater than EFI_PAGE_SIZE RUNTIME_PAGE_ALLOCATION_GRANULARITY" > + ); > + > #endif > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > index 3205732ede17..ed3908b9768e 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -1411,6 +1411,17 @@ CoreInternalAllocatePages ( > Alignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY; > } > > + // > + // the heap guard system does not support non-EFI_PAGE_SIZE alignments > + // architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY > + // will have the runtime and ACPI memory regions unguarded. OSes do not > + // map guard pages anyway, so this is a minimal loss. Not guarding prevents > + // alignment mismatches > + // > + if (Alignment != EFI_PAGE_SIZE) { > + NeedGuard = FALSE; > + } > + > if (Type == AllocateAddress) { > if ((*Memory & (Alignment - 1)) != 0) { > return EFI_NOT_FOUND; > diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c > index 716dd045f9fd..beed5f814510 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c > @@ -380,6 +380,17 @@ CoreAllocatePoolI ( > Granularity = DEFAULT_PAGE_ALLOCATION_GRANULARITY; > } > > + // > + // the heap guard system does not support non-EFI_PAGE_SIZE alignments > + // architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY > + // will have the runtime and ACPI memory regions unguarded. OSes do not > + // map guard pages anyway, so this is a minimal loss. Not guarding prevents > + // alignment mismatches > + // > + if (Granularity != EFI_PAGE_SIZE) { > + NeedGuard = FALSE; > + } > + > // > // Adjust the size by the pool header & tail overhead > // > -- > 2.40.1 > > > > ------------ > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#115474): https://edk2.groups.io/g/devel/message/115474 > Mute This Topic: https://groups.io/mt/104364784/5717338 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org] > ------------ > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115490): https://edk2.groups.io/g/devel/message/115490 Mute This Topic: https://groups.io/mt/104364784/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-