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

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

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