Oliver: This change reverts the changes done in AdjustPoolHeadA(). It matches the allocation logic. I think this change is good. Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn> I am also OK to merge this change for the stable tag 202308.
Besides, AdjustPoolHeadA() implementation has the extra logic " Size = ALIGN_VALUE (Size, 8);". Seemly, this logic is not required, because Size has aligned by ALIGN_VARIABLE before enter into AdjustPoolHeadA. Thanks Liming > -----邮件原件----- > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Leif Lindholm > 发送时间: 2023年8月15日 18:58 > 收件人: Ard Biesheuvel <a...@kernel.org>; Oliver Smith-Denny > <o...@linux.microsoft.com>; Liming Gao <gaolim...@byosoft.com.cn> > 抄送: devel@edk2.groups.io; Jian J Wang <jian.j.w...@intel.com>; Dandan > Bi <dandan...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > 'Andrew Fish' <af...@apple.com> > 主题: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: > HeapGuard: Don't Assume Pool Head Allocated In First Page > > On 2023-08-09 22:51, Ard Biesheuvel wrote: > > 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? > > Bugfix, submitted during soft freeze. I think it can go in. > *but* this affects !AARCH64 also - need a reaction from MdeModulePkg > maintainers. > > Acked-by: Leif Lindholm <quic_llind...@quicinc.com> > > >> --- > >> 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 (#107777): https://edk2.groups.io/g/devel/message/107777 Mute This Topic: https://groups.io/mt/100771741/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
