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 <o...@linux.microsoft.com> > 发送时间: 2023年8月18日 1:05 > 收件人: gaoliming <gaolim...@byosoft.com.cn>; devel@edk2.groups.io; > quic_llind...@quicinc.com; 'Ard Biesheuvel' <a...@kernel.org> > 抄送: '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> > 主题: 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 > <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 > > 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@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 (#107882): https://edk2.groups.io/g/devel/message/107882 Mute This Topic: https://groups.io/mt/100833709/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
