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 A77B62098C8B6 for ; Thu, 19 Jul 2018 06:01:46 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 02216401C090; Thu, 19 Jul 2018 13:01:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-95.rdu2.redhat.com [10.10.120.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3CA831C5B3; Thu, 19 Jul 2018 13:01:37 +0000 (UTC) To: "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" , "Yao, Jiewen" , "Zeng, Star" References: <20180713055357.4196-1-jian.j.wang@intel.com> <2b6a7d47-8d72-0a06-a41e-945226d57bf4@redhat.com> From: Laszlo Ersek Message-ID: <8cd51df3-bad0-9ae1-a43a-8e06966cd87b@redhat.com> Date: Thu, 19 Jul 2018 15:01:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 19 Jul 2018 13:01:46 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 19 Jul 2018 13:01:46 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Jul 2018 13:01:46 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/19/18 11:07, Wang, Jian J wrote: > Hi Laszlo, > > Do you have more comments? Or can you give a r-b? Struggling with my workload, will try to come back to this soon. Thanks Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Wednesday, July 18, 2018 10:36 AM >> To: Laszlo Ersek ; edk2-devel@lists.01.org >> Cc: Dong, Eric ; Yao, Jiewen ; >> Zeng, Star >> Subject: RE: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode >> >> Hi Laszlo, >> >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Tuesday, July 17, 2018 10:37 PM >>> To: Wang, Jian J ; edk2-devel@lists.01.org >>> Cc: Dong, Eric ; Yao, Jiewen ; >>> Zeng, Star >>> Subject: Re: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode >>> >>> On 07/13/18 07:53, Jian J Wang wrote: >>>> Current IsInSmm() method makes use of gEfiSmmBase2ProtocolGuid.InSmm() >>> to >>>> check if current processor is in SMM mode or not. But this is not correct >>>> because gEfiSmmBase2ProtocolGuid.InSmm() can only detect if the caller is >>>> running in SMRAM or from SMM driver. It cannot guarantee if the caller is >>>> running in SMM mode. >>> >>> Wow. This is the exact difference which I asked about, in my question >>> (9b), and I was assured that InSmm() actually determined whether we were >>> executing in Management Mode (meaning the processor operating mode). >>> >>> http://mid.mail- >>> >> archive.com/0C09AFA07DD0434D9E2A0C6AEB0483103BB55B46@shsmsx102.cc >>> r.corp.intel.com >>> >>> (Scroll down in that message to see my original question (9b).) >>> >>> So why doesn't Star's explanation, from the original thread, apply >>> ultimately? >>> >> >> We did many tests for this and didn't found any issue. So I took a risk. (I thought >> a very precise check of SMM mode is hard and time consuming.) >> >>>> Because SMM mode will load its own page table, adding >>>> an extra check of saved DXE page table base address against current CR3 >>>> register value can help to get the correct answer for sure (in SMM mode or >>>> not in SMM mode). >>> >>> So, apparently, the PI spec offers no standard way for a platform module >>> to determine whether it runs in Management Mode, despite protocol member >>> being called "InSmm". Do we need a PI spec ECR for introducing the >>> needed facility? >>> >>> Alternatively, the PI spec might already intend to specify that, but the >>> edk2 implementation doesn't do what the PI spec requires. >>> >>> Which one is the case? >>> >> >> The implementation conforms to the spec. It's my misunderstanding. But it's >> true >> that there's no specific protocol API to determine if it's in SMM mode or not. >> >>>> >>>> This is an issue caused by check-in at >>>> >>>> d106cf71eabaacff63c14626a4a87346b93074dd >>> >>> I disagree; I think the issue was introduced in commit 2a1408d1d739 >>> ("UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode", >>> 2018-06-19). >>> >> >> You're right. Thanks for pointing this out. >> >>> >>> How did you encounter / find this issue? >>> >> >> I didn't find it. The issue came to me. In other words, I think it's random and hard >> to reproduce it. Maybe a subtle change in boot sequence will hide it away. >> >>>> >>>> Cc: Eric Dong >>>> Cc: Laszlo Ersek >>>> Cc: Jiewen Yao >>>> Cc: Star Zeng >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Jian J Wang >>>> --- >>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>> index 850eed60e7..df021798c0 100644 >>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>> @@ -136,7 +136,14 @@ IsInSmm ( >>>> mSmmBase2->InSmm (mSmmBase2, &InSmm); >>>> } >>>> >>>> - return InSmm; >>>> + // >>>> + // mSmmBase2->InSmm() can only detect if the caller is running in SMRAM >>>> + // or from SMM driver. It cannot tell if the caller is running in SMM mode. >>>> + // Check page table base address to guarantee that because SMM mode >>> willl >>>> + // load its own page table. >>>> + // >>>> + return (InSmm && >>>> + mPagingContext.ContextData.X64.PageTableBase != >>> (UINT64)AsmReadCr3()); >>>> } >>>> >>>> /** >>>> >>> >>> Shouldn't we consider Ia32.PageTableBase when that's appropriate? From >>> "UefiCpuPkg/CpuDxe/CpuPageTable.h": >>> >>> typedef struct { >>> UINT32 PageTableBase; >>> UINT32 Reserved; >>> UINT32 Attributes; >>> } PAGE_TABLE_LIB_PAGING_CONTEXT_IA32; >>> >>> typedef struct { >>> UINT64 PageTableBase; >>> UINT32 Attributes; >>> } PAGE_TABLE_LIB_PAGING_CONTEXT_X64; >>> >>> typedef union { >>> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32 Ia32; >>> PAGE_TABLE_LIB_PAGING_CONTEXT_X64 X64; >>> } PAGE_TABLE_LIB_PAGING_CONTEXT_DATA; >>> >>> The Ia32/X64 structure types are not packed, and even if they were, I >>> wouldn't think we should rely on "Reserved" being zero. >>> >> >> mPagingContext is zero-ed at each update in GetCurrentPagingContext(). >> I think it should be no problem to use X64. >> >>> >>> All in all, I think that determining whether the processor is operating >>> in Management Mode (regardless of where in RAM the processor is >>> executing code from) is a core functionality that should be offered as a >>> central service, not just an internal CpuDxe function. I think we need >>> either a PI spec addition, or at least an EDKII extension protocol. It's >>> obvious that the InSmm() behavior is unclear to developers! (Me >>> included, of course.) >>> >> >> I agree. >> >>> Thanks, >>> Laszlo