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.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>;
> Zeng, Star <star.z...@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...@intel.com>
> >
> >
> >>
> >>> -----Original Message-----
> >>> From: Ni, Ruiyu
> >>> Sent: Tuesday, January 30, 2018 12:38 PM
> >>> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> >>> Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric
> >>> <eric.d...@intel.com>; Yao,
> >>> Jiewen <jiewen....@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.z...@intel.com>
> >>>> Cc: Eric Dong <eric.d...@intel.com>
> >>>> Cc: Jiewen Yao <jiewen....@intel.com>
> >>>> Cc: Ruiyu Ni <ruiyu...@intel.com>
> >>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>> Signed-off-by: Jian J Wang <jian.j.w...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
