From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2038A22135D3C for ; Tue, 6 Mar 2018 20:25:56 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2018 20:32:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,434,1515484800"; d="scan'208";a="25841830" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.44]) ([10.239.9.44]) by fmsmga002.fm.intel.com with ESMTP; 06 Mar 2018 20:32:09 -0800 To: "Wu, Hao A" , "Yao, Jiewen" , "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Zeng, Star" , "Dong, Eric" References: <20180306133303.14772-1-hao.a.wu@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AADF3FF@shsmsx102.ccr.corp.intel.com> From: "Ni, Ruiyu" Message-ID: <42416a1d-9f60-402f-421f-49a576f1aed5@Intel.com> Date: Wed, 7 Mar 2018 12:32:09 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Mar 2018 04:25:57 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 3/7/2018 12:28 PM, Wu, Hao A wrote: > Hi Ray, > > Below are the answers to your feedbacks: > >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Wednesday, March 07, 2018 12:14 PM >> To: Wang, Jian J; Ni, Ruiyu; Wu, Hao A; edk2-devel@lists.01.org >> Cc: Zeng, Star; Dong, Eric >> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack >> guard >> >> I think the original patch is fine. >> >> StackBase is already checked by using ASSERT before. >>> + ASSERT ((StackBase & EFI_PAGE_MASK) == 0); >> >> MemMap entry must be page aligned. >> >> No additional check is required here. >> >> Thank you >> Yao Jiewen >> >> >>> -----Original Message----- >>> From: Wang, Jian J >>> Sent: Wednesday, March 7, 2018 11:40 AM >>> To: Ni, Ruiyu ; Wu, Hao A ; >>> edk2-devel@lists.01.org >>> Cc: Zeng, Star ; Dong, Eric ; Yao, >>> Jiewen >>> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack >> guard >>> >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Ni, Ruiyu >>>> Sent: Wednesday, March 07, 2018 11:17 AM >>>> To: Wu, Hao A ; edk2-devel@lists.01.org >>>> Cc: Wang, Jian J ; Zeng, Star >> ; >>>> Dong, Eric ; Yao, Jiewen >>>> Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack >>>> guard >>>> >>>> On 3/6/2018 9:33 PM, Hao Wu wrote: >>>>> V2 changes: >>>>> >>>>> A. Use Hoblib APIs to get the base of stack from Hob. >>>>> B. Remove unnecessary local variable used in function >>>>> InitializeDxeNxMemoryProtectionPolicy(). >>>>> >>>>> V1 history: >>>>> >>>>> 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 series will 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 >>>>> Cc: Star Zeng >>>>> Cc: Eric Dong >>>>> Cc: Jiewen Yao >>>>> Cc: Ruiyu Ni >>>>> >>>>> Hao Wu (2): >>>>> MdeModulePkg/Core: Refine handling NULL detection in NX setting >>>>> MdeModulePkg/Core: Fix feature conflict between NX and Stack guard >>>>> >>>>> MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- >>>>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 >>>> +++++++++++++++++++++++---- >>>>> 2 files changed, 67 insertions(+), 11 deletions(-) >>>>> >>>> >>>> if (MemoryMapEntry->PhysicalStart == 0 && >>>> PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { >>>> >>>> ASSERT (MemoryMapEntry->NumberOfPages > 0); >>>> // >>>> // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer >>>> detection is >>>> // enabled. >>>> // >>>> [Ray] 1. I prefer to move the above comments before the "if (...)". > > Yes. I agree that moving the comments block out of the 'if' statement is > more readable. I will update according to your suggestion when I pushing > the changes. > >>>> >>>> SetUefiImageMemoryAttributes ( >>>> 0, >>>> EFI_PAGES_TO_SIZE (1), >>>> EFI_MEMORY_RP | Attributes); >>>> } >>>> >>>> if (StackBase != 0 && >>>> (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); >>>> [Ray] 2. The StackBase is directly used here. So do we need to check >>>> whether it's page aligned? Do we need to check whether the range >>>> [StackBase, StackBase + 4KB) is inside the MemoryMapEntry? >>>> } >>> >>> If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for >>> StackBase >>> should make sure all the conditions you mentioned, but not here. >>> > > Just as Jiewen mentioned in the previous reply. An ASSERT: > ASSERT ((StackBase & EFI_PAGE_MASK) == 0); > > is added to ensure the stack base fetched from Hob is page-size aligned. > > And the below check: > (StackBase >= MemoryMapEntry->PhysicalStart && > StackBase < MemoryMapEntry->PhysicalStart + > LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT)) > > together with the fact that MemMap is page aligned (also mentioned by > Jiewen) ensures that the first page of the stack is cover by the memory > range of the MemMap. I agree. Thanks for the explanation. Please move the comments location when checking in the patch. > > Best Regards, > Hao Wu > >>>> >>>> -- >>>> Thanks, >>>> Ray -- Thanks, Ray