From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 E5B4C2257C2CA for ; Sun, 4 Mar 2018 21:00:41 -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 fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Mar 2018 21:06:52 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,426,1515484800"; d="scan'208";a="25178819" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.44]) ([10.239.9.44]) by fmsmga002.fm.intel.com with ESMTP; 04 Mar 2018 21:06:51 -0800 To: Laszlo Ersek , Jian J Wang , edk2-devel@lists.01.org Cc: Eric Dong References: <20180302055839.18248-1-jian.j.wang@intel.com> <31960905-5140-ea20-aa02-38eff5be3cba@redhat.com> <04d43b92-5697-2561-e672-600caa518141@Intel.com> <89616b7a-ddc8-a92b-0b30-a9bc9d1c5a8f@redhat.com> From: "Ni, Ruiyu" Message-ID: Date: Mon, 5 Mar 2018 13:06:51 +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: <89616b7a-ddc8-a92b-0b30-a9bc9d1c5a8f@redhat.com> Subject: Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory 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: Mon, 05 Mar 2018 05:00:42 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit On 3/3/2018 11:10 PM, Laszlo Ersek wrote: > Hi Ray, > > On 03/02/18 12:57, Ni, Ruiyu wrote: >> On 3/2/2018 7:45 PM, Laszlo Ersek wrote: >>> On 03/02/18 06:58, Jian J Wang wrote: >>>> if PcdDxeNxMemoryProtectionPolicy is enabled for EfiReservedMemoryType >>>> of memory, #PF will be triggered for each APs after ExitBootServices >>>> in SCRT test. The root cause is that AP wakeup code executed at that >>>> time is stored in memory of type EfiReservedMemoryType (referenced by >>>> global mReservedApLoopFunc), which is marked as non-executable. >>>> >>>> This patch fixes this issue by setting memory of mReservedApLoopFunc to >>>> be executable immediately after allocation. >>>> >>>> Cc: Ruiyu Ni >>>> Cc: Eric Dong >>>> Cc: Laszlo Ersek >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Jian J Wang >>>> --- >>>>   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 15 +++++++++++++++ >>>>   1 file changed, 15 insertions(+) >>>> >>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> index fd2317924f..5fcb08677c 100644 >>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> @@ -399,6 +399,21 @@ InitMpGlobalData ( >>>>                      &Address >>>>                      ); >>>>     ASSERT_EFI_ERROR (Status); >>>> + >>>> +  // >>>> +  // Make sure that the buffer memory is executable. >>>> +  // >>>> +  Status = gDS->GetMemorySpaceDescriptor (Address, &MemDesc); >>>> +  if (!EFI_ERROR (Status)) { >>>> +    gDS->SetMemorySpaceAttributes ( >>>> +           Address, >>>> +           EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES ( >>>> +             CpuMpData->AddressMap.RelocateApLoopFuncSize >>>> +             )), >>>> +           MemDesc.Attributes & (~EFI_MEMORY_XP) >>>> +           ); >>>> +  } >>>> + >>>>     mReservedApLoopFunc = (VOID *) (UINTN) Address; >>>>     ASSERT (mReservedApLoopFunc != NULL); >>>>     mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE >>>> (EFI_SIZE_TO_PAGES (ApSafeBufferSize)); >>>> >>> >>> Honestly, I see little point in the "Dxe Nx Memory Protection Policy" >>> when we then override it *every time* it gets in our way. >>> "RelocateApLoopFuncSize" is likely significantly smaller than a full >>> page, so we're making a good chunk of the "safe stack(s)" executable too. >>> >>> Anyway, can you perhaps check BIT0 (standing for EfiReservedMemoryType) >>> in PcdDxeNxMemoryProtectionPolicy, to see if the above hack is necessary? >>> >>> Thanks >>> Laszlo >>> >> >> Checking PCD is not very good I think. > > I'll look at v2 next week, just a short comment now: I don't understand > why you are opposed to the PCD check. Reserved memory is generally > expected to be executable, and the issue surfaces *precisely* when > reserved memory is marked as noexec in the PCD in question. That's > exactly the reason why the above logic is needed. Sorry I didn't see this comments. Standing from CPU owner's perspective, I don't like to check PCD here. Or, to be straight, I want to have least knowledge of any extra non-spec defined interfaces. > > Approach it from this side: if I was reading the code (without the PCD > check), I would ask myself, "why are we checking for noexec here? we > just allocated this chunk of reserved memory from normal system memory. > It should be executable already". > > So, I think the PCD check is somewhat important functionally, and quite > important for documentation purposes. And it's a lot better than adding > a comment. > >> If checking is really needed, how about check MemDesc.Attributes >> EFI_MEMORY_XP bit? > > I think that would check for the symptom, not for the root cause. To a > person reading the code, it doesn't provide any more information than > the current code. "Okay, it's not executable, so we mark it executable > manually. But why isn't it executable in the first place? We just > allocated it from system memory."This code is for AP to sleep, even at runtime, before OS takes control of AP. So making it as BS code may cause issues. > > Thanks, > Laszlo > -- Thanks, Ray