Re: [PATCH 2/2] MdeModulePkg/Core: Fix feature conflict between NX and Stack guard

["Wu, Hao A" <hao.a.wu@intel.com>]

Got it, I will send out a V2 of the series according to the comments.

Best Regards,
Hao Wu


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Tuesday, March 06, 2018 9:11 PM
> To: Yao, Jiewen; Zeng, Star; Wu, Hao A; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Dong, Eric
> Subject: RE: [PATCH 2/2] MdeModulePkg/Core: Fix feature conflict between NX
> and Stack guard
> 
> BTW: I don't think "StackBaseFound" is really needed.
> 
> We can use ASSERT (StackBase != 0); directly. :-)
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao,
> > Jiewen
> > Sent: Tuesday, March 6, 2018 9:05 PM
> > To: Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/Core: Fix feature conflict
> > between NX and Stack guard
> >
> > Agree.
> >
> > With this update, reviewed-by: Jiewen.yao@intel.com
> >
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Tuesday, March 6, 2018 8:16 PM
> > > To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Dong, Eric
> <eric.dong@intel.com>;
> > > Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng,
> > Star
> > > <star.zeng@intel.com>
> > > Subject: RE: [PATCH 2/2] MdeModulePkg/Core: Fix feature conflict between
> > NX
> > > and Stack guard
> > >
> > > A quick minor comment.
> > > GetHobList() could be used instead of EfiGetSystemConfigurationTable
> > > (&gEfiHobListGuid, &HobList).
> > >
> > > Thanks,
> > > Star
> > > -----Original Message-----
> > > From: Wu, Hao A
> > > Sent: Tuesday, March 6, 2018 8:11 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>;
> > > 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 2/2] MdeModulePkg/Core: Fix feature conflict between NX
> > and
> > > Stack guard
> > >
> > > If enabled, NX memory protection feature will mark some types of active
> > > memory as NX (non-executable), which includes the first page of the stack.
> > > This will overwrite the attributes of the first page of the stack if the stack
> guard
> > > feature is also enabled.
> > >
> > > The solution is to override the attributes setting to the first page of the
> stack by
> > > adding back the 'EFI_MEMORY_RP' attribute when the stack guard feature
> is
> > > enabled.
> > >
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > 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: Hao Wu <hao.a.wu@intel.com>
> > > ---
> > >  MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
> > >  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 61
> > > +++++++++++++++++++++++++++
> > >  2 files changed, 64 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > index 7334780326..d2e7360ed4 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > @@ -3,7 +3,7 @@
> > >  #
> > >  #  It provides an implementation of DXE Core that is compliant with DXE
> CIS.
> > >  #
> > > -#  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> > > +#  Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > +reserved.<BR>
> > >  #  This program and the accompanying materials  #  are licensed and
> > made
> > > available under the terms and conditions of the BSD License  #  which
> > > accompanies this distribution.  The full text of the license may be found at
> > @@
> > > -130,6 +130,7 @@
> > >    gEfiPropertiesTableGuid                       ##
> > > SOMETIMES_PRODUCES   ## SystemTable
> > >    gEfiMemoryAttributesTableGuid                 ##
> > > SOMETIMES_PRODUCES   ## SystemTable
> > >    gEfiEndOfDxeEventGroupGuid                    ##
> > > SOMETIMES_CONSUMES   ## Event
> > > +  gEfiHobMemoryAllocStackGuid                   ##
> > > SOMETIMES_CONSUMES   ## SystemTable
> > >
> > >  [Ppis]
> > >    gEfiVectorHandoffInfoPpiGuid                  ## UNDEFINED # HOB
> > > @@ -198,6 +199,7 @@
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType
> > > ## CONSUMES
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType
> > > ## CONSUMES
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> > > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > > ## CONSUMES
> > >
> > >  # [Hob]
> > >  # RESOURCE_DESCRIPTOR   ## CONSUMES
> > > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > > b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > > index a2ea445eef..a6de22d3af 100644
> > > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > > @@ -801,6 +801,11 @@ InitializeDxeNxMemoryProtectionPolicy (
> > >    UINT64                            Attributes;
> > >    LIST_ENTRY                        *Link;
> > >    EFI_GCD_MAP_ENTRY                 *Entry;
> > > +  VOID                              *HobList;
> > > +  EFI_PEI_HOB_POINTERS              Hob;
> > > +  EFI_HOB_MEMORY_ALLOCATION         *MemoryHob;
> > > +  EFI_PHYSICAL_ADDRESS              StackBase;
> > > +  BOOLEAN                           StackBaseFound;
> > >
> > >    //
> > >    // Get the EFI memory map.
> > > @@ -832,6 +837,45 @@ InitializeDxeNxMemoryProtectionPolicy (
> > >    } while (Status == EFI_BUFFER_TOO_SMALL);
> > >    ASSERT_EFI_ERROR (Status);
> > >
> > > +  StackBase = 0;
> > > +  StackBaseFound = FALSE;
> > > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > > +    //
> > > +    // Get the base of stack from Hob.
> > > +    //
> > > +    Status = EfiGetSystemConfigurationTable (&gEfiHobListGuid, &HobList);
> > > +    if (!EFI_ERROR (Status)) {
> > > +      for (Hob.Raw = HobList; !END_OF_HOB_LIST (Hob); Hob.Raw =
> > > GET_NEXT_HOB (Hob)) {
> > > +        if (GET_HOB_TYPE(Hob) ==
> > EFI_HOB_TYPE_MEMORY_ALLOCATION)
> > > {
> > > +          MemoryHob = Hob.MemoryAllocation;
> > > +          if (CompareGuid(&gEfiHobMemoryAllocStackGuid,
> > > &MemoryHob->AllocDescriptor.Name)) {
> > > +            DEBUG ((
> > > +              DEBUG_INFO,
> > > +              "%a: StackBase = 0x%016lx  StackSize = 0x%016lx\n",
> > > +              __FUNCTION__,
> > > +              MemoryHob->AllocDescriptor.MemoryBaseAddress,
> > > +              MemoryHob->AllocDescriptor.MemoryLength
> > > +              ));
> > > +
> > > +            StackBase =
> > MemoryHob->AllocDescriptor.MemoryBaseAddress;
> > > +            //
> > > +            // Ensure the base of the stack is page-size aligned.
> > > +            //
> > > +            ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
> > > +            StackBaseFound = TRUE;
> > > +            break;
> > > +          }
> > > +        }
> > > +      }
> > > +    }
> > > +
> > > +    //
> > > +    // Ensure the base of stack can be found from Hob when stack guard is
> > > +    // enabled.
> > > +    //
> > > +    ASSERT (StackBaseFound);
> > > +  }
> > > +
> > >    DEBUG ((
> > >      DEBUG_INFO,
> > >      "%a: applying strict permissions to active memory regions\n", @@
> > -864,6
> > > +908,23 @@ InitializeDxeNxMemoryProtectionPolicy (
> > >            EFI_PAGES_TO_SIZE (1),
> > >            EFI_MEMORY_RP | Attributes);
> > >        }
> > > +
> > > +      if (StackBaseFound &&
> > > +          (StackBase >= MemoryMapEntry->PhysicalStart &&
> > > +           StackBase <  MemoryMapEntry->PhysicalStart +
> > > +                        LShiftU64 (MemoryMapEntry->NumberOfPages,
> > > EFI_PAGE_SHIFT)) &&
> > > +          PcdGetBool (PcdCpuStackGuard)) {
> > > +
> > > +        //
> > > +        // Add EFI_MEMORY_RP attribute for the first page of the stack if
> > > stack
> > > +        // guard is enabled.
> > > +        //
> > > +        SetUefiImageMemoryAttributes (
> > > +          StackBase,
> > > +          EFI_PAGES_TO_SIZE (1),
> > > +          EFI_MEMORY_RP | Attributes);
> > > +      }
> > > +
> > >      }
> > >      MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> > > DescriptorSize);
> > >    }
> > > --
> > > 2.12.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel