* [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection @ 2018-01-29 11:09 Jian J Wang 2018-01-30 2:09 ` Yao, Jiewen 2018-01-30 4:38 ` Ni, Ruiyu 0 siblings, 2 replies; 8+ messages in thread From: Jian J Wang @ 2018-01-29 11:09 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Eric Dong, Jiewen Yao, Ruiyu Ni 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-01-29 11:09 [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection Jian J Wang @ 2018-01-30 2:09 ` Yao, Jiewen 2018-02-01 1:14 ` Wang, Jian J 2018-01-30 4:38 ` Ni, Ruiyu 1 sibling, 1 reply; 8+ messages in thread From: Yao, Jiewen @ 2018-01-30 2:09 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Zeng, Star, Dong, Eric, Ni, Ruiyu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-01-30 2:09 ` Yao, Jiewen @ 2018-02-01 1:14 ` Wang, Jian J 0 siblings, 0 replies; 8+ messages in thread From: Wang, Jian J @ 2018-02-01 1:14 UTC (permalink / raw) To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Zeng, Star, Dong, Eric, Ni, Ruiyu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-01-29 11:09 [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection Jian J Wang 2018-01-30 2:09 ` Yao, Jiewen @ 2018-01-30 4:38 ` Ni, Ruiyu 2018-02-01 1:17 ` Wang, Jian J 1 sibling, 1 reply; 8+ messages in thread From: Ni, Ruiyu @ 2018-01-30 4:38 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Star Zeng, Eric Dong, Jiewen Yao 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-01-30 4:38 ` Ni, Ruiyu @ 2018-02-01 1:17 ` Wang, Jian J 2018-02-01 5:33 ` Ni, Ruiyu 0 siblings, 1 reply; 8+ messages in thread From: Wang, Jian J @ 2018-02-01 1:17 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Zeng, Star, Dong, Eric, Yao, Jiewen 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-02-01 1:17 ` Wang, Jian J @ 2018-02-01 5:33 ` Ni, Ruiyu 2018-02-01 5:54 ` Ni, Ruiyu 0 siblings, 1 reply; 8+ messages in thread From: Ni, Ruiyu @ 2018-02-01 5:33 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Zeng, Star, Dong, Eric, Yao, Jiewen 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-02-01 5:33 ` Ni, Ruiyu @ 2018-02-01 5:54 ` Ni, Ruiyu 2018-02-01 5:59 ` Wang, Jian J 0 siblings, 1 reply; 8+ messages in thread From: Ni, Ruiyu @ 2018-02-01 5:54 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection 2018-02-01 5:54 ` Ni, Ruiyu @ 2018-02-01 5:59 ` Wang, Jian J 0 siblings, 0 replies; 8+ messages in thread From: Wang, Jian J @ 2018-02-01 5:59 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-02-01 5:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-29 11:09 [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection Jian J Wang 2018-01-30 2:09 ` Yao, Jiewen 2018-02-01 1:14 ` Wang, Jian J 2018-01-30 4:38 ` Ni, Ruiyu 2018-02-01 1:17 ` Wang, Jian J 2018-02-01 5:33 ` Ni, Ruiyu 2018-02-01 5:54 ` Ni, Ruiyu 2018-02-01 5:59 ` Wang, Jian J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox