[edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[Oliver Smith-Denny]

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>
---
 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 (#107656): https://edk2.groups.io/g/devel/message/107656
Mute This Topic: https://groups.io/mt/100652659/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[Ard Biesheuvel]

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?

> ---
>  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 (#107658): https://edk2.groups.io/g/devel/message/107658
Mute This Topic: https://groups.io/mt/100652659/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[Leif Lindholm]

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 (#107765): https://edk2.groups.io/g/devel/message/107765
Mute This Topic: https://groups.io/mt/100755729/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[gaoliming via groups.io]

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
> -----邮件原件-----
> 发件人: 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 (#107777): https://edk2.groups.io/g/devel/message/107777
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[Oliver Smith-Denny]

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]
-=-=-=-=-=-=-=-=-=-=-=-

回复: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[gaoliming via groups.io]

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

回复: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page

[gaoliming via groups.io]

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' <osde@linux.microsoft.com>;
> devel@edk2.groups.io; quic_llindhol@quicinc.com; 'Ard Biesheuvel'
> <ardb@kernel.org>
> 抄送: '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
> 
> 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@linux.microsoft.com>
> > 发送时间: 2023年8月18日 1:05
> > 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io;
> > quic_llindhol@quicinc.com; 'Ard Biesheuvel' <ardb@kernel.org>
> > 抄送: '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>
> > 主题: 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@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 (#107883): https://edk2.groups.io/g/devel/message/107883
Mute This Topic: https://groups.io/mt/100833735/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-