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 467F4740032 for ; Wed, 9 Aug 2023 21:51:31 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=BP02zprzMJLz1bQigZDoYFSCqvejZ1w3X4sBzCs/Rxc=; 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=1691617890; v=1; b=VcGz+8NKsgMCrqqxJJ0j47/dv0AeIUybjH6WkVwj4nIO1WDDeKP259Fo/IplYCqVes1fpqZ5 3DYQQZR9M/tpLNlpljOuemFLv6FLlaNW4ozbHIBQc8uNwOz/bmzmycNOVQhYHciNgYzomtmtrE3 1jwj7zyVqTsGzGU6p9cgAY2w= X-Received: by 127.0.0.2 with SMTP id 1DHYYY7687511xb9D1cXYXKC; Wed, 09 Aug 2023 14:51:30 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.4026.1691617889331044516 for ; Wed, 09 Aug 2023 14:51:29 -0700 X-Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A03BB641D9 for ; Wed, 9 Aug 2023 21:51:28 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 06E2BC433C8 for ; Wed, 9 Aug 2023 21:51:28 +0000 (UTC) X-Received: by mail-lf1-f48.google.com with SMTP id 2adb3069b0e04-4fe0eb0ca75so305478e87.2 for ; Wed, 09 Aug 2023 14:51:27 -0700 (PDT) X-Gm-Message-State: kfprhlUVqwdfJgfNlG0JhSydx7686176AA= X-Google-Smtp-Source: AGHT+IF5Xq8emcirpaE9l3Y5s+CZx/OfUvwt/ujBxIsQhBL6pebXBFiYmQU26R/kc+ZN7cXI2kx/dPGYJ29TSzVnNIg= X-Received: by 2002:a05:6512:3c97:b0:4f8:711b:18b0 with SMTP id h23-20020a0565123c9700b004f8711b18b0mr300256lfv.3.1691617885999; Wed, 09 Aug 2023 14:51:25 -0700 (PDT) MIME-Version: 1.0 References: <20230809213457.18664-1-osde@linux.microsoft.com> In-Reply-To: <20230809213457.18664-1-osde@linux.microsoft.com> From: "Ard Biesheuvel" Date: Wed, 9 Aug 2023 23:51:14 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page To: Oliver Smith-Denny , Liming Gao Cc: devel@edk2.groups.io, Leif Lindholm , Jian J Wang , Dandan Bi 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=VcGz+8NK; 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 Wed, 9 Aug 2023 at 23:35, Oliver Smith-Denny wrote: > > Currently, HeapGuard, when in the GuardAlignedToTail mode, assumes that > the pool head has been allocated in the first page of memory that was > allocated. This is not the case for ARM64 platforms when allocating > runtime pools, as RUNTIME_PAGE_ALLOCATION_GRANULARITY is 64k, unlike > X64, which has RUNTIME_PAGE_ALLOCATION_GRANULARITY as 4k. > > When a runtime pool is allocated on ARM64, the minimum number of pages > allocated is 16, to match the runtime granularity. When a small pool is > allocated and GuardAlignedToTail is true, HeapGuard instructs the pool > head to be placed as (MemoryAllocated + EFI_PAGES_TO_SIZE(Number of Pages) > - SizeRequiredForPool). > > This gives this scenario: > > |Head Guard|Large Free Number of Pages|PoolHead|TailGuard| > > When this pool goes to be freed, HeapGuard instructs the pool code to > free from (PoolHead & ~EFI_PAGE_MASK). However, this assumes that the > PoolHead is in the first page allocated, which as shown above is not true > in this case. For the 4k granularity case (i.e. where the correct number of > pages are allocated for this pool), this logic does work. > > In this failing case, HeapGuard then instructs the pool code to free 16 > (or more depending) pages from the page the pool head was allocated on, > which as seen above means we overrun the pool and attempt to free memory > far past the pool. We end up running into the tail guard and getting an > access flag fault. > > This causes ArmVirtQemu to fail to boot with an access flag fault when > GuardAlignedToTail is set to true (and pool guard enabled for runtime > memory). It should also cause all ARM64 platforms to fail in this > configuration, for exactly the same reason, as this is core code making > the assumption. > > This patch removes HeapGuard's assumption that the pool head is allocated > on the first page and instead undoes the same logic that HeapGuard did > when allocating the pool head in the first place. > > With this patch in place, ArmVirtQemu boots with GuardAlignedToTail > set to true (and when it is false, also). > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4521 > Github PR: https://github.com/tianocore/edk2/pull/4731 > > Cc: Leif Lindholm > Cc: Ard Biesheuvel > Cc: Jian J Wang > Cc: Liming Gao > Cc: Dandan Bi > > Signed-off-by: Oliver Smith-Denny Thanks a lot for fixing this - I ran into this a while ago but didn't quite figure out what exactly was going wrong, although the runtime allocation granularity was obviously a factor here. Reviewed-by: Ard Biesheuvel Can we include this in the stable tag please? > --- > MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 7 ++++++- > MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 14 +++++++++++--- > MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- > 3 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h > index 9a32b4dd51d5..24b4206c0e02 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h > @@ -378,12 +378,17 @@ AdjustPoolHeadA ( > Get the page base address according to pool head address. > > @param[in] Memory Head address of pool to free. > + @param[in] NoPages Number of pages actually allocated. > + @param[in] Size Size of memory requested. > + (plus pool head/tail overhead) > > @return Address of pool head. > **/ > VOID * > AdjustPoolHeadF ( > - IN EFI_PHYSICAL_ADDRESS Memory > + IN EFI_PHYSICAL_ADDRESS Memory, > + IN UINTN NoPages, > + IN UINTN Size > ); > > /** > diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > index 9377f620c5a5..0c0ca61872b4 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > @@ -1037,12 +1037,17 @@ AdjustPoolHeadA ( > Get the page base address according to pool head address. > > @param[in] Memory Head address of pool to free. > + @param[in] NoPages Number of pages actually allocated. > + @param[in] Size Size of memory requested. > + (plus pool head/tail overhead) > > @return Address of pool head. > **/ > VOID * > AdjustPoolHeadF ( > - IN EFI_PHYSICAL_ADDRESS Memory > + IN EFI_PHYSICAL_ADDRESS Memory, > + IN UINTN NoPages, > + IN UINTN Size > ) > { > if ((Memory == 0) || ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0)) { > @@ -1053,9 +1058,12 @@ AdjustPoolHeadF ( > } > > // > - // Pool head is put near the tail Guard > + // Pool head is put near the tail Guard. We need to exactly undo the addition done in AdjustPoolHeadA > + // because we may not have allocated the pool head on the first allocated page, since we are aligned to > + // the tail and on some architectures, the runtime page allocation granularity is > one page. So we allocate > + // more pages than we need and put the pool head somewhere past the first page. > // > - return (VOID *)(UINTN)(Memory & ~EFI_PAGE_MASK); > + return (VOID *)(UINTN)(Memory + Size - EFI_PAGES_TO_SIZE (NoPages)); > } > > /** > diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c > index b20cbfdedbab..716dd045f9fd 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c > @@ -783,7 +783,7 @@ CoreFreePoolI ( > NoPages = EFI_SIZE_TO_PAGES (Size) + EFI_SIZE_TO_PAGES (Granularity) - 1; > NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1); > if (IsGuarded) { > - Head = AdjustPoolHeadF ((EFI_PHYSICAL_ADDRESS)(UINTN)Head); > + Head = AdjustPoolHeadF ((EFI_PHYSICAL_ADDRESS)(UINTN)Head, NoPages, Size); > CoreFreePoolPagesWithGuard ( > Pool->MemoryType, > (EFI_PHYSICAL_ADDRESS)(UINTN)Head, > -- > 2.40.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107658): https://edk2.groups.io/g/devel/message/107658 Mute This Topic: https://groups.io/mt/100652659/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-