[PATCH v2 0/1] SMM memory management: Inspect memory guarded with pool headers

[PATCH v2 0/1] SMM memory management: Inspect memory guarded with pool headers

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3488

This patch series is a follow up of previous submission:
https://edk2.groups.io/g/devel/message/87618

v2 patches mainly focus on feedback for commits submitted in v1 patches:
a. Added reviewed-by tags;

Patch v2 branch: https://github.com/kuqin12/edk2/tree/mm_zero_sized_pool_v2

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Kun Qin (1):
  MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

 MdeModulePkg/Core/PiSmmCore/Pool.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.35.1.windows.2

[PATCH v2 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3488

Current free pool routine from PiSmmCore will inspect memory guard status
for target buffer without considering pool headers. This could lead to
`IsMemoryGuarded` function to return incorrect results.

In that sense, allocating a 0 sized pool could cause an allocated buffer
directly points into a guard page, which is legal. However, trying to
free this pool will cause the routine changed in this commit to read XP
pages, which leads to page fault.

This change will inspect memory guarded with pool headers. This can avoid
errors when a pool content happens to be on a page boundary.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
---

Notes:
    v2:
    - Added reviewed-by tag [Jian]
    - Added reviewed-by tag [Liming]

 MdeModulePkg/Core/PiSmmCore/Pool.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c b/MdeModulePkg/Core/PiSmmCore/Pool.c
index 96ebe811c669..e1ff40a8ea55 100644
--- a/MdeModulePkg/Core/PiSmmCore/Pool.c
+++ b/MdeModulePkg/Core/PiSmmCore/Pool.c
@@ -382,11 +382,6 @@ SmmInternalFreePool (
     return EFI_INVALID_PARAMETER;
   }
 
-  MemoryGuarded = IsHeapGuardEnabled () &&
-                  IsMemoryGuarded ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer);
-  HasPoolTail = !(MemoryGuarded &&
-                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0));
-
   FreePoolHdr = (FREE_POOL_HEADER *)((POOL_HEADER *)Buffer - 1);
   ASSERT (FreePoolHdr->Header.Signature == POOL_HEAD_SIGNATURE);
   ASSERT (!FreePoolHdr->Header.Available);
@@ -394,6 +389,11 @@ SmmInternalFreePool (
     return EFI_INVALID_PARAMETER;
   }
 
+  MemoryGuarded = IsHeapGuardEnabled () &&
+                  IsMemoryGuarded ((EFI_PHYSICAL_ADDRESS)(UINTN)FreePoolHdr);
+  HasPoolTail = !(MemoryGuarded &&
+                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0));
+
   if (HasPoolTail) {
     PoolTail = HEAD_TO_TAIL (&FreePoolHdr->Header);
     ASSERT (PoolTail->Signature == POOL_TAIL_SIGNATURE);
-- 
2.35.1.windows.2

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[Kun Qin]

Hi maintainers,

This patch was reviewed and sent a while back, could you please help me 
to merge in this change, if no further feedback?

Thanks in advance,
Kun

On 4/25/2022 5:47 PM, Kun Qin via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3488
>
> Current free pool routine from PiSmmCore will inspect memory guard status
> for target buffer without considering pool headers. This could lead to
> `IsMemoryGuarded` function to return incorrect results.
>
> In that sense, allocating a 0 sized pool could cause an allocated buffer
> directly points into a guard page, which is legal. However, trying to
> free this pool will cause the routine changed in this commit to read XP
> pages, which leads to page fault.
>
> This change will inspect memory guarded with pool headers. This can avoid
> errors when a pool content happens to be on a page boundary.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> ---
>
> Notes:
>      v2:
>      - Added reviewed-by tag [Jian]
>      - Added reviewed-by tag [Liming]
>
>   MdeModulePkg/Core/PiSmmCore/Pool.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c b/MdeModulePkg/Core/PiSmmCore/Pool.c
> index 96ebe811c669..e1ff40a8ea55 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Pool.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Pool.c
> @@ -382,11 +382,6 @@ SmmInternalFreePool (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> -  MemoryGuarded = IsHeapGuardEnabled () &&
> -                  IsMemoryGuarded ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer);
> -  HasPoolTail = !(MemoryGuarded &&
> -                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0));
> -
>     FreePoolHdr = (FREE_POOL_HEADER *)((POOL_HEADER *)Buffer - 1);
>     ASSERT (FreePoolHdr->Header.Signature == POOL_HEAD_SIGNATURE);
>     ASSERT (!FreePoolHdr->Header.Available);
> @@ -394,6 +389,11 @@ SmmInternalFreePool (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> +  MemoryGuarded = IsHeapGuardEnabled () &&
> +                  IsMemoryGuarded ((EFI_PHYSICAL_ADDRESS)(UINTN)FreePoolHdr);
> +  HasPoolTail = !(MemoryGuarded &&
> +                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0));
> +
>     if (HasPoolTail) {
>       PoolTail = HEAD_TO_TAIL (&FreePoolHdr->Header);
>       ASSERT (PoolTail->Signature == POOL_TAIL_SIGNATURE);

回复: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[gaoliming]

Kun:
  This patch is reviewed before soft feature freeze. I agree to merge it for this stable tag. 

  Here is PR https://github.com/tianocore/edk2/pull/2881

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
> 发送时间: 2022年5月13日 8:23
> 收件人: devel@edk2.groups.io
> 抄送: Jiewen Yao <jiewen.yao@intel.com>; Eric Dong <eric.dong@intel.com>;
> Ray Ni <ray.ni@intel.com>; Jian J Wang <jian.j.wang@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>
> 主题: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: PiSmmCore: Inspect
> memory guarded with pool headers
> 
> Hi maintainers,
> 
> This patch was reviewed and sent a while back, could you please help me
> to merge in this change, if no further feedback?
> 
> Thanks in advance,
> Kun
> 
> On 4/25/2022 5:47 PM, Kun Qin via groups.io wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3488
> >
> > Current free pool routine from PiSmmCore will inspect memory guard status
> > for target buffer without considering pool headers. This could lead to
> > `IsMemoryGuarded` function to return incorrect results.
> >
> > In that sense, allocating a 0 sized pool could cause an allocated buffer
> > directly points into a guard page, which is legal. However, trying to
> > free this pool will cause the routine changed in this commit to read XP
> > pages, which leads to page fault.
> >
> > This change will inspect memory guarded with pool headers. This can avoid
> > errors when a pool content happens to be on a page boundary.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >
> > Signed-off-by: Kun Qin <kuqin12@gmail.com>
> > Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> > Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> > ---
> >
> > Notes:
> >      v2:
> >      - Added reviewed-by tag [Jian]
> >      - Added reviewed-by tag [Liming]
> >
> >   MdeModulePkg/Core/PiSmmCore/Pool.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c
> b/MdeModulePkg/Core/PiSmmCore/Pool.c
> > index 96ebe811c669..e1ff40a8ea55 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/Pool.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/Pool.c
> > @@ -382,11 +382,6 @@ SmmInternalFreePool (
> >       return EFI_INVALID_PARAMETER;
> >     }
> >
> > -  MemoryGuarded = IsHeapGuardEnabled () &&
> > -                  IsMemoryGuarded
> ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer);
> > -  HasPoolTail = !(MemoryGuarded &&
> > -                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) ==
> 0));
> > -
> >     FreePoolHdr = (FREE_POOL_HEADER *)((POOL_HEADER *)Buffer - 1);
> >     ASSERT (FreePoolHdr->Header.Signature ==
> POOL_HEAD_SIGNATURE);
> >     ASSERT (!FreePoolHdr->Header.Available);
> > @@ -394,6 +389,11 @@ SmmInternalFreePool (
> >       return EFI_INVALID_PARAMETER;
> >     }
> >
> > +  MemoryGuarded = IsHeapGuardEnabled () &&
> > +                  IsMemoryGuarded
> ((EFI_PHYSICAL_ADDRESS)(UINTN)FreePoolHdr);
> > +  HasPoolTail = !(MemoryGuarded &&
> > +                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) ==
> 0));
> > +
> >     if (HasPoolTail) {
> >       PoolTail = HEAD_TO_TAIL (&FreePoolHdr->Header);
> >       ASSERT (PoolTail->Signature == POOL_TAIL_SIGNATURE);
> 
> 
> 
> 

Re: 回复: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[Kun Qin]

Thank you for the help, Liming!

On 5/12/2022 5:38 PM, gaoliming wrote:
> Kun:
>    This patch is reviewed before soft feature freeze. I agree to merge it for this stable tag.
>
>    Here is PR https://github.com/tianocore/edk2/pull/2881
>
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
>> 发送时间: 2022年5月13日 8:23
>> 收件人: devel@edk2.groups.io
>> 抄送: Jiewen Yao <jiewen.yao@intel.com>; Eric Dong <eric.dong@intel.com>;
>> Ray Ni <ray.ni@intel.com>; Jian J Wang <jian.j.wang@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>
>> 主题: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: PiSmmCore: Inspect
>> memory guarded with pool headers
>>
>> Hi maintainers,
>>
>> This patch was reviewed and sent a while back, could you please help me
>> to merge in this change, if no further feedback?
>>
>> Thanks in advance,
>> Kun
>>
>> On 4/25/2022 5:47 PM, Kun Qin via groups.io wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3488
>>>
>>> Current free pool routine from PiSmmCore will inspect memory guard status
>>> for target buffer without considering pool headers. This could lead to
>>> `IsMemoryGuarded` function to return incorrect results.
>>>
>>> In that sense, allocating a 0 sized pool could cause an allocated buffer
>>> directly points into a guard page, which is legal. However, trying to
>>> free this pool will cause the routine changed in this commit to read XP
>>> pages, which leads to page fault.
>>>
>>> This change will inspect memory guarded with pool headers. This can avoid
>>> errors when a pool content happens to be on a page boundary.
>>>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>>>
>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
>>> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>>> ---
>>>
>>> Notes:
>>>       v2:
>>>       - Added reviewed-by tag [Jian]
>>>       - Added reviewed-by tag [Liming]
>>>
>>>    MdeModulePkg/Core/PiSmmCore/Pool.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c
>> b/MdeModulePkg/Core/PiSmmCore/Pool.c
>>> index 96ebe811c669..e1ff40a8ea55 100644
>>> --- a/MdeModulePkg/Core/PiSmmCore/Pool.c
>>> +++ b/MdeModulePkg/Core/PiSmmCore/Pool.c
>>> @@ -382,11 +382,6 @@ SmmInternalFreePool (
>>>        return EFI_INVALID_PARAMETER;
>>>      }
>>>
>>> -  MemoryGuarded = IsHeapGuardEnabled () &&
>>> -                  IsMemoryGuarded
>> ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer);
>>> -  HasPoolTail = !(MemoryGuarded &&
>>> -                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) ==
>> 0));
>>> -
>>>      FreePoolHdr = (FREE_POOL_HEADER *)((POOL_HEADER *)Buffer - 1);
>>>      ASSERT (FreePoolHdr->Header.Signature ==
>> POOL_HEAD_SIGNATURE);
>>>      ASSERT (!FreePoolHdr->Header.Available);
>>> @@ -394,6 +389,11 @@ SmmInternalFreePool (
>>>        return EFI_INVALID_PARAMETER;
>>>      }
>>>
>>> +  MemoryGuarded = IsHeapGuardEnabled () &&
>>> +                  IsMemoryGuarded
>> ((EFI_PHYSICAL_ADDRESS)(UINTN)FreePoolHdr);
>>> +  HasPoolTail = !(MemoryGuarded &&
>>> +                  ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) ==
>> 0));
>>> +
>>>      if (HasPoolTail) {
>>>        PoolTail = HEAD_TO_TAIL (&FreePoolHdr->Header);
>>>        ASSERT (PoolTail->Signature == POOL_TAIL_SIGNATURE);
>>
>>
>>
>
>
>
>
> 
>
>