Oliver: https://github.com/tianocore/edk2/pull/4751 is created to merge this patch.
Thanks Liming > -----邮件原件----- > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 gaoliming via > groups.io > 发送时间: 2023年8月19日 10:45 > 收件人: 'Oliver Smith-Denny' <o...@linux.microsoft.com>; > 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> > 主题: 回复: 回复: 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 <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 (#107883): https://edk2.groups.io/g/devel/message/107883 Mute This Topic: https://groups.io/mt/100833735/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
