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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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