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

[PATCH v1 0/1] SMM memory management: 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.

i.e. A 0-sized pool that is configured to be near tail guard page, it
could cause the returned region points into a guard page, which is legal.
However, trying to free this 0 sized pool will cause `IsMemoryGuarded` to
access guard page, 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.

Patch v1 branch: https://github.com/kuqin12/edk2/tree/mm_zero_sized_pool

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 v1 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>
---
 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

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

[gaoliming]

Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
> 发送时间: 2022年3月16日 12:00
> 收件人: 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>
> 主题: [edk2-devel] [PATCH v1 1/1] MdeModulePkg: PiSmmCore: Inspect
> memory guarded with pool headers
> 
> 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>
> ---
>  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 v1 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[Kun Qin]

Thanks, Liming.

SMM owners/authors,

Could you please also review the original issue and this patch to 
provide feedback?

Thanks,
Kun

On 3/17/2022 6:20 PM, gaoliming wrote:
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
>> 发送时间: 2022年3月16日 12:00
>> 收件人: 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>
>> 主题: [edk2-devel] [PATCH v1 1/1] MdeModulePkg: PiSmmCore: Inspect
>> memory guarded with pool headers
>>
>> 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>
>> ---
>>   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 v1 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[Kun Qin]

Hi SMM maintainers,

A gentle ping on this. Could you please provide some feedback on the fix 
below for allocating 0 sized pool when heap guard it on?

Thanks in advance,
Kun

On 3/28/2022 2:57 PM, Kun Qin wrote:
> Thanks, Liming.
>
> SMM owners/authors,
>
> Could you please also review the original issue and this patch to 
> provide feedback?
>
> Thanks,
> Kun
>
> On 3/17/2022 6:20 PM, gaoliming wrote:
>> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>>
>>> -----邮件原件-----
>>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
>>> 发送时间: 2022年3月16日 12:00
>>> 收件人: 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>
>>> 主题: [edk2-devel] [PATCH v1 1/1] MdeModulePkg: PiSmmCore: Inspect
>>> memory guarded with pool headers
>>>
>>> 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>
>>> ---
>>>   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 v1 1/1] MdeModulePkg: PiSmmCore: Inspect memory guarded with pool headers

[Wang, Jian J]

It looks good to me. Thanks for fixing it.

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
> Sent: Wednesday, March 16, 2022 12:00 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel] [PATCH v1 1/1] MdeModulePkg: PiSmmCore: Inspect
> memory guarded with pool headers
> 
> 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>
> ---
>  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
> 
> 
> 
> 
>