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

["Wang, Jian J" <jian.j.wang@intel.com>]

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