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@linux.microsoft.com>]

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@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 <ardb@kernel.org>; Oliver Smith-Denny
>> <osde@linux.microsoft.com>; Liming Gao <gaoliming@byosoft.com.cn>
>> 抄送: devel@edk2.groups.io; Jian J Wang <jian.j.wang@intel.com>; Dandan
>> Bi <dandan.bi@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> 'Andrew Fish' <afish@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@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@quicinc.com>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>>>> Cc: Dandan Bi <dandan.bi@intel.com>
>>>>
>>>> Signed-off-by: Oliver Smith-Denny <osde@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@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@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-