回复: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page
gaoliming via groups.io
gaoliming=byosoft.com.cn at groups.io
Sat Aug 19 02:49:53 UTC 2023
Oliver:
https://github.com/tianocore/edk2/pull/4751 is created to merge this patch.
Thanks
Liming
> -----邮件原件-----
> 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 gaoliming via
> groups.io
> 发送时间: 2023年8月19日 10:45
> 收件人: 'Oliver Smith-Denny' <osde at linux.microsoft.com>;
> devel at edk2.groups.io; quic_llindhol at quicinc.com; 'Ard Biesheuvel'
> <ardb at kernel.org>
> 抄送: 'Jian J Wang' <jian.j.wang at intel.com>; 'Dandan Bi'
> <dandan.bi at intel.com>; 'Kinney, Michael D' <michael.d.kinney at intel.com>;
> 'Andrew Fish' <afish at apple.com>
> 主题: 回复: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1]
> MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page
>
> Oliver:
> This patch is not required to be updated. You can send another for this clean
> up in Allocate after the stable tag.
>
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: Oliver Smith-Denny <osde at linux.microsoft.com>
> > 发送时间: 2023年8月18日 1:05
> > 收件人: gaoliming <gaoliming at byosoft.com.cn>; devel at edk2.groups.io;
> > quic_llindhol at quicinc.com; 'Ard Biesheuvel' <ardb at kernel.org>
> > 抄送: 'Jian J Wang' <jian.j.wang at intel.com>; 'Dandan Bi'
> > <dandan.bi at intel.com>; 'Kinney, Michael D'
> <michael.d.kinney at intel.com>;
> > 'Andrew Fish' <afish at apple.com>
> > 主题: Re: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1]
> > MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First
> Page
> >
> > On 8/15/2023 6:40 PM, gaoliming wrote:
> > > Oliver:
> > > This change reverts the changes done in AdjustPoolHeadA(). It
> matches
> > the allocation logic. I think this change is good. Reviewed-by: Liming Gao
> > <gaoliming at 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
> >
> > Thanks for the review, Liming. Looking at the alignment code, I agree,
> > the ALIGN_VALUE doesn't seem to be needed. Do you want me to send a v2
> > with that dropped or take the patch as is? Looks like we have the
> > required reviewers and probably no further folks reviewing.
> >
> > Thanks,
> > Oliver
> >
> > >> -----邮件原件-----
> > >> 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Leif
> > Lindholm
> > >> 发送时间: 2023年8月15日 18:58
> > >> 收件人: Ard Biesheuvel <ardb at kernel.org>; Oliver Smith-Denny
> > >> <osde at linux.microsoft.com>; Liming Gao <gaoliming at byosoft.com.cn>
> > >> 抄送: devel at edk2.groups.io; Jian J Wang <jian.j.wang at intel.com>;
> > Dandan
> > >> Bi <dandan.bi at intel.com>; Kinney, Michael D
> > <michael.d.kinney at intel.com>;
> > >> 'Andrew Fish' <afish at 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
> > >>> <osde at 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_llindhol at quicinc.com>
> > >>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
> > >>>> Cc: Jian J Wang <jian.j.wang at intel.com>
> > >>>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> > >>>> Cc: Dandan Bi <dandan.bi at intel.com>
> > >>>>
> > >>>> Signed-off-by: Oliver Smith-Denny <osde at 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 <ardb at 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_llindhol at 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 (#107883): https://edk2.groups.io/g/devel/message/107883
Mute This Topic: https://groups.io/mt/100833735/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list