Re: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page
Oliver Smith-Denny
osde at linux.microsoft.com
Thu Aug 17 17:05:23 UTC 2023
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 (#107836): https://edk2.groups.io/g/devel/message/107836
Mute This Topic: https://groups.io/mt/100771741/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