[PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Jian J Wang]

If enabled, NX memory protection feature will mark all free memory as
NX (non-executable), including page 0. This will overwrite the attributes
of page 0 if NULL pointer detection feature is also enabled and then
compromise the functionality of it. The solution is skipping the NX
attributes setting to page 0 if NULL pointer detection feature is enabled.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 862593f562..150167bf66 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
 
     Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
     if (Attributes != 0) {
-      SetUefiImageMemoryAttributes (
-        MemoryMapEntry->PhysicalStart,
-        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
-        Attributes);
+      if (MemoryMapEntry->PhysicalStart == 0 &&
+          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
+        //
+        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
+        // overwritten.
+        //
+        SetUefiImageMemoryAttributes (
+          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
+          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
+          Attributes);
+      } else {
+        SetUefiImageMemoryAttributes (
+          MemoryMapEntry->PhysicalStart,
+          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
+          Attributes);
+      }
     }
     MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
   }
-- 
2.14.1.windows.1

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Yao, Jiewen]

Hi Jian
May I know how we handle MemoryMapEntry->NumberOfPages is 1?
The lengh will be 0 in that case. Should we add additional check?

> +        SetUefiImageMemoryAttributes (
> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> EFI_PAGE_SHIFT),
> +          Attributes);

> -----Original Message-----
> From: Wang, Jian J
> Sent: Monday, January 29, 2018 7:10 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
> NULL detection
> 
> If enabled, NX memory protection feature will mark all free memory as
> NX (non-executable), including page 0. This will overwrite the attributes
> of page 0 if NULL pointer detection feature is also enabled and then
> compromise the functionality of it. The solution is skipping the NX
> attributes setting to page 0 if NULL pointer detection feature is enabled.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
> ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 862593f562..150167bf66 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
> 
>      Attributes = GetPermissionAttributeForMemoryType
> (MemoryMapEntry->Type);
>      if (Attributes != 0) {
> -      SetUefiImageMemoryAttributes (
> -        MemoryMapEntry->PhysicalStart,
> -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> -        Attributes);
> +      if (MemoryMapEntry->PhysicalStart == 0 &&
> +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> +        //
> +        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
> +        // overwritten.
> +        //
> +        SetUefiImageMemoryAttributes (
> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> EFI_PAGE_SHIFT),
> +          Attributes);
> +      } else {
> +        SetUefiImageMemoryAttributes (
> +          MemoryMapEntry->PhysicalStart,
> +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> +          Attributes);
> +      }
>      }
>      MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> DescriptorSize);
>    }
> --
> 2.14.1.windows.1

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Ni, Ruiyu]

On 1/29/2018 7:09 PM, Jian J Wang wrote:
> If enabled, NX memory protection feature will mark all free memory as
> NX (non-executable), including page 0. This will overwrite the attributes
> of page 0 if NULL pointer detection feature is also enabled and then
> compromise the functionality of it. The solution is skipping the NX
> attributes setting to page 0 if NULL pointer detection feature is enabled.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 862593f562..150167bf66 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
>   
>       Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
>       if (Attributes != 0) {
> -      SetUefiImageMemoryAttributes (
> -        MemoryMapEntry->PhysicalStart,
> -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> -        Attributes);
> +      if (MemoryMapEntry->PhysicalStart == 0 &&
> +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> +        //
> +        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
> +        // overwritten.
> +        //
> +        SetUefiImageMemoryAttributes (
> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
> +          Attributes);
> +      } else {
> +        SetUefiImageMemoryAttributes (
> +          MemoryMapEntry->PhysicalStart,
> +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> +          Attributes);
> +      }
>       }
>       MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
>     }
> 
Does this bug expose an API-level issue?
SetUefiImageMemoryAttributes () should also accept a Mask value?

-- 
Thanks,
Ray

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Wang, Jian J]

gCpu->SetMemoryAttributes() called by SetUefiImageMemoryAttributes() will return
without any problem if Length is 0.

Regards,
Jian


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Tuesday, January 30, 2018 10:09 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
> NULL detection
> 
> Hi Jian
> May I know how we handle MemoryMapEntry->NumberOfPages is 1?
> The lengh will be 0 in that case. Should we add additional check?
> 
> > +        SetUefiImageMemoryAttributes (
> > +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> > +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> > EFI_PAGE_SHIFT),
> > +          Attributes);
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Monday, January 29, 2018 7:10 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
> > NULL detection
> >
> > If enabled, NX memory protection feature will mark all free memory as
> > NX (non-executable), including page 0. This will overwrite the attributes
> > of page 0 if NULL pointer detection feature is also enabled and then
> > compromise the functionality of it. The solution is skipping the NX
> > attributes setting to page 0 if NULL pointer detection feature is enabled.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
> > ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > index 862593f562..150167bf66 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
> >
> >      Attributes = GetPermissionAttributeForMemoryType
> > (MemoryMapEntry->Type);
> >      if (Attributes != 0) {
> > -      SetUefiImageMemoryAttributes (
> > -        MemoryMapEntry->PhysicalStart,
> > -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> > -        Attributes);
> > +      if (MemoryMapEntry->PhysicalStart == 0 &&
> > +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> > +        //
> > +        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
> > +        // overwritten.
> > +        //
> > +        SetUefiImageMemoryAttributes (
> > +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> > +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> > EFI_PAGE_SHIFT),
> > +          Attributes);
> > +      } else {
> > +        SetUefiImageMemoryAttributes (
> > +          MemoryMapEntry->PhysicalStart,
> > +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> > +          Attributes);
> > +      }
> >      }
> >      MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> > DescriptorSize);
> >    }
> > --
> > 2.14.1.windows.1

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Wang, Jian J]

You're right. Using a mask or separating the API into two (SetMemoryAttributes/ClearMemoryAttributes)
is much better and can avoid many potential issues.

Regards,
Jian


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, January 30, 2018 12:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
> NULL detection
> 
> On 1/29/2018 7:09 PM, Jian J Wang wrote:
> > If enabled, NX memory protection feature will mark all free memory as
> > NX (non-executable), including page 0. This will overwrite the attributes
> > of page 0 if NULL pointer detection feature is also enabled and then
> > compromise the functionality of it. The solution is skipping the NX
> > attributes setting to page 0 if NULL pointer detection feature is enabled.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
> ++++++++++++++++----
> >   1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > index 862593f562..150167bf66 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
> >
> >       Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry-
> >Type);
> >       if (Attributes != 0) {
> > -      SetUefiImageMemoryAttributes (
> > -        MemoryMapEntry->PhysicalStart,
> > -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> > -        Attributes);
> > +      if (MemoryMapEntry->PhysicalStart == 0 &&
> > +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> > +        //
> > +        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
> > +        // overwritten.
> > +        //
> > +        SetUefiImageMemoryAttributes (
> > +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> > +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
> > +          Attributes);
> > +      } else {
> > +        SetUefiImageMemoryAttributes (
> > +          MemoryMapEntry->PhysicalStart,
> > +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> > +          Attributes);
> > +      }
> >       }
> >       MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> DescriptorSize);
> >     }
> >
> Does this bug expose an API-level issue?
> SetUefiImageMemoryAttributes () should also accept a Mask value?
> 
> --
> Thanks,
> Ray

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Ni, Ruiyu]

On 2/1/2018 9:17 AM, Wang, Jian J wrote:
> You're right. Using a mask or separating the API into two (SetMemoryAttributes/ClearMemoryAttributes)
> is much better and can avoid many potential issues.
> 
> Regards,
> Jian
> 

For now the patch is good enough to leave NULL pointer detection
feature enabled.

Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>


> 
>> -----Original Message-----
>> From: Ni, Ruiyu
>> Sent: Tuesday, January 30, 2018 12:38 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
>> Jiewen <jiewen.yao@intel.com>
>> Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
>> NULL detection
>>
>> On 1/29/2018 7:09 PM, Jian J Wang wrote:
>>> If enabled, NX memory protection feature will mark all free memory as
>>> NX (non-executable), including page 0. This will overwrite the attributes
>>> of page 0 if NULL pointer detection feature is also enabled and then
>>> compromise the functionality of it. The solution is skipping the NX
>>> attributes setting to page 0 if NULL pointer detection feature is enabled.
>>>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>> ---
>>>    MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
>> ++++++++++++++++----
>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> index 862593f562..150167bf66 100644
>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
>>>
>>>        Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry-
>>> Type);
>>>        if (Attributes != 0) {
>>> -      SetUefiImageMemoryAttributes (
>>> -        MemoryMapEntry->PhysicalStart,
>>> -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>>> -        Attributes);
>>> +      if (MemoryMapEntry->PhysicalStart == 0 &&
>>> +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
>>> +        //
>>> +        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
>>> +        // overwritten.
>>> +        //
>>> +        SetUefiImageMemoryAttributes (
>>> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
>>> +          Attributes);
>>> +      } else {
>>> +        SetUefiImageMemoryAttributes (
>>> +          MemoryMapEntry->PhysicalStart,
>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>>> +          Attributes);
>>> +      }
>>>        }
>>>        MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>> DescriptorSize);
>>>      }
>>>
>> Does this bug expose an API-level issue?
>> SetUefiImageMemoryAttributes () should also accept a Mask value?
>>
>> --
>> Thanks,
>> Ray


-- 
Thanks,
Ray

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Ni, Ruiyu]

On 2/1/2018 1:33 PM, Ni, Ruiyu wrote:
> On 2/1/2018 9:17 AM, Wang, Jian J wrote:
>> You're right. Using a mask or separating the API into two 
>> (SetMemoryAttributes/ClearMemoryAttributes)
>> is much better and can avoid many potential issues.
>>
>> Regards,
>> Jian
>>
> 
> For now the patch is good enough to leave NULL pointer detection
> feature enabled.
> 
> Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>
> 
> 
>>
>>> -----Original Message-----
>>> From: Ni, Ruiyu
>>> Sent: Tuesday, January 30, 2018 12:38 PM
>>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric 
>>> <eric.dong@intel.com>; Yao,
>>> Jiewen <jiewen.yao@intel.com>
>>> Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between 
>>> NX and
>>> NULL detection
>>>
>>> On 1/29/2018 7:09 PM, Jian J Wang wrote:
>>>> If enabled, NX memory protection feature will mark all free memory as
>>>> NX (non-executable), including page 0. This will overwrite the 
>>>> attributes
>>>> of page 0 if NULL pointer detection feature is also enabled and then
>>>> compromise the functionality of it. The solution is skipping the NX
>>>> attributes setting to page 0 if NULL pointer detection feature is 
>>>> enabled.
>>>>
>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>>> ---
>>>>    MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
>>> ++++++++++++++++----
>>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> index 862593f562..150167bf66 100644
>>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
>>>>
>>>>        Attributes = GetPermissionAttributeForMemoryType 
>>>> (MemoryMapEntry-
>>>> Type);
>>>>        if (Attributes != 0) {
>>>> -      SetUefiImageMemoryAttributes (
>>>> -        MemoryMapEntry->PhysicalStart,
>>>> -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>>>> -        Attributes);
>>>> +      if (MemoryMapEntry->PhysicalStart == 0 &&
>>>> +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
>>>> +        //
>>>> +        // Skip page 0 if NULL pointer detection is enabled to 
>>>> avoid attributes
>>>> +        // overwritten.
>>>> +        //

By the way, could you please add an assertion here?
ASSERT (MemoryMapEntry->NumberOfPages != 0);
>>>> +        SetUefiImageMemoryAttributes (
>>>> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
>>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, 
>>>> EFI_PAGE_SHIFT),
>>>> +          Attributes);
>>>> +      } else {
>>>> +        SetUefiImageMemoryAttributes (
>>>> +          MemoryMapEntry->PhysicalStart,
>>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>>>> +          Attributes);
>>>> +      }
>>>>        }
>>>>        MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>>> DescriptorSize);
>>>>      }
>>>>
>>> Does this bug expose an API-level issue?
>>> SetUefiImageMemoryAttributes () should also accept a Mask value?
>>>
>>> -- 
>>> Thanks,
>>> Ray
> 
> 


-- 
Thanks,
Ray

Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Wang, Jian J]

Ok. Thanks for the comments.

Regards,
Jian

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, February 01, 2018 1:54 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between
> NX and NULL detection
> 
> On 2/1/2018 1:33 PM, Ni, Ruiyu wrote:
> > On 2/1/2018 9:17 AM, Wang, Jian J wrote:
> >> You're right. Using a mask or separating the API into two
> >> (SetMemoryAttributes/ClearMemoryAttributes)
> >> is much better and can avoid many potential issues.
> >>
> >> Regards,
> >> Jian
> >>
> >
> > For now the patch is good enough to leave NULL pointer detection
> > feature enabled.
> >
> > Reviewed-by: Ruiyu Ni <ruiyu.ni@Intel.com>
> >
> >
> >>
> >>> -----Original Message-----
> >>> From: Ni, Ruiyu
> >>> Sent: Tuesday, January 30, 2018 12:38 PM
> >>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> >>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> >>> <eric.dong@intel.com>; Yao,
> >>> Jiewen <jiewen.yao@intel.com>
> >>> Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between
> >>> NX and
> >>> NULL detection
> >>>
> >>> On 1/29/2018 7:09 PM, Jian J Wang wrote:
> >>>> If enabled, NX memory protection feature will mark all free memory as
> >>>> NX (non-executable), including page 0. This will overwrite the
> >>>> attributes
> >>>> of page 0 if NULL pointer detection feature is also enabled and then
> >>>> compromise the functionality of it. The solution is skipping the NX
> >>>> attributes setting to page 0 if NULL pointer detection feature is
> >>>> enabled.
> >>>>
> >>>> Cc: Star Zeng <star.zeng@intel.com>
> >>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> >>>> ---
> >>>>    MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
> >>> ++++++++++++++++----
> >>>>    1 file changed, 16 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >>>> index 862593f562..150167bf66 100644
> >>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >>>> @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
> >>>>
> >>>>        Attributes = GetPermissionAttributeForMemoryType
> >>>> (MemoryMapEntry-
> >>>> Type);
> >>>>        if (Attributes != 0) {
> >>>> -      SetUefiImageMemoryAttributes (
> >>>> -        MemoryMapEntry->PhysicalStart,
> >>>> -        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> >>>> -        Attributes);
> >>>> +      if (MemoryMapEntry->PhysicalStart == 0 &&
> >>>> +          PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> >>>> +        //
> >>>> +        // Skip page 0 if NULL pointer detection is enabled to
> >>>> avoid attributes
> >>>> +        // overwritten.
> >>>> +        //
> 
> By the way, could you please add an assertion here?
> ASSERT (MemoryMapEntry->NumberOfPages != 0);
> >>>> +        SetUefiImageMemoryAttributes (
> >>>> +          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> >>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> >>>> EFI_PAGE_SHIFT),
> >>>> +          Attributes);
> >>>> +      } else {
> >>>> +        SetUefiImageMemoryAttributes (
> >>>> +          MemoryMapEntry->PhysicalStart,
> >>>> +          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> >>>> +          Attributes);
> >>>> +      }
> >>>>        }
> >>>>        MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> >>> DescriptorSize);
> >>>>      }
> >>>>
> >>> Does this bug expose an API-level issue?
> >>> SetUefiImageMemoryAttributes () should also accept a Mask value?
> >>>
> >>> --
> >>> Thanks,
> >>> Ray
> >
> >
> 
> 
> --
> Thanks,
> Ray