On Wed, 9 Aug 2023 at 23:35, Oliver Smith-Denny
<o...@linux.microsoft.com> 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 <quic_llind...@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Dandan Bi <dandan...@intel.com>
>
> Signed-off-by: Oliver Smith-Denny <o...@linux.microsoft.com>
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 <a...@kernel.org>
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
