From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 6FEEF209574ED for ; Sat, 3 Mar 2018 07:04:22 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5D6B4818595C; Sat, 3 Mar 2018 15:10:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-176.rdu2.redhat.com [10.10.120.176]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6407A2144B21; Sat, 3 Mar 2018 15:10:31 +0000 (UTC) To: "Ni, Ruiyu" , 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> From: Laszlo Ersek Message-ID: <89616b7a-ddc8-a92b-0b30-a9bc9d1c5a8f@redhat.com> Date: Sat, 3 Mar 2018 16:10:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <04d43b92-5697-2561-e672-600caa518141@Intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Sat, 03 Mar 2018 15:10:32 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Sat, 03 Mar 2018 15:10:32 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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: Sat, 03 Mar 2018 15:04:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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. 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." Thanks, Laszlo