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 17834AC0D27 for ; Mon, 11 Mar 2024 14:01:03 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=qxtDYPV9vVxfmO29TCL/nbFSVAX71PURTRfrgRK5l5I=; 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=20240206; t=1710165662; v=1; b=BRI85bClfC5mZibXRHO5jBKIDpr3sQklmNUm/9CbsJ922gcSoTME68BgndwYbKDrmBIUFE2b qSl3u3EZLV6Lk2qFDSoT2C8sI6Ugs0fOOaIPcaijMqIA1GgJhz13JgbnsBlij239+zdnI03Q1EN c+08x0LZ8kqfshSpRWRrbkvZ0Kc2DHYup62wiJLFhAX3EEFYWsaYm9HNS7wOf7gBGlPEFxxCCjP /zlwaPM4xEmp7C4WlJ9ogmJk8zrr6QbF50XAcUNMYJEFz3PvJP7ML348tCt3/h38Cam3OLzIR58 t4AqaW9+FVqBKyL3iPkj1wE6A20OwmyCLLCwTzxKuveyQ== X-Received: by 127.0.0.2 with SMTP id EuqeYY7687511xfo8N29DRe6; Mon, 11 Mar 2024 07:01:02 -0700 X-Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by mx.groups.io with SMTP id smtpd.web11.63645.1710165661535034875 for ; Mon, 11 Mar 2024 07:01:02 -0700 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id B302ACE10B7 for ; Mon, 11 Mar 2024 14:00:58 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id F0F80C43399 for ; Mon, 11 Mar 2024 14:00:57 +0000 (UTC) X-Received: by mail-lf1-f48.google.com with SMTP id 2adb3069b0e04-512ed314881so3791821e87.2 for ; Mon, 11 Mar 2024 07:00:57 -0700 (PDT) X-Gm-Message-State: EazTkUmpDK6T0Ag49zY6X1f2x7686176AA= X-Google-Smtp-Source: AGHT+IFHG6o1ozAcJ5/BBndkRJ6+3yMF/WIGVB1j4R9oKgfgYvF975vm8+CZQ4Vp0IuPJhvTS7jozV6u4OMuudTsuNM= X-Received: by 2002:a05:6512:601:b0:513:37a1:ae60 with SMTP id b1-20020a056512060100b0051337a1ae60mr4315541lfe.34.1710165656086; Mon, 11 Mar 2024 07:00:56 -0700 (PDT) MIME-Version: 1.0 References: <20240309190559.28677-1-osde@linux.microsoft.com> <20240309190559.28677-4-osde@linux.microsoft.com> In-Reply-To: <20240309190559.28677-4-osde@linux.microsoft.com> From: "Ard Biesheuvel" Date: Mon, 11 Mar 2024 07:01:02 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][PATCH v3 3/3] MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types To: Oliver Smith-Denny Cc: devel@edk2.groups.io, 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=20240206 header.b=BRI85bCl; 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 On Sat, 9 Mar 2024 at 20:06, Oliver Smith-Denny wrote: > > Currently, there are multiple issues when page or pool guards are > allocated for runtime 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 case, > we break UEFI spec 2.10 section 2.3.6 for AARCH64, which states that > each 64k page for runtime memory regions may not have mixed memory > attributes, which pushing the guard pages inside would create. 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. > > 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. 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/5382 > > 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(+) > Reviewed-by: Ard Biesheuvel > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index a2cd83345f5b..a82dedc070df 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, > + # EfiReservedMemoryType, 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 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, > + # EfiReservedMemoryType, 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..578e85746585 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, EfiReservedMemoryType, > +// 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) & 0x461) == 0) && > + ((FixedPcdGet64 (PcdHeapGuardPoolType) & 0x461) == 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 cd201d36a389..26584648c236 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 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 ccfce8c5f959..72293e6dfe40 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 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 (#116628): https://edk2.groups.io/g/devel/message/116628 Mute This Topic: https://groups.io/mt/104832607/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-