Hi Lazlo, Thank you very much for the effort. We do have an emergency situation.
(1) Agree. Filed a bug (https://bugzilla.tianocore.org/show_bug.cgi?id=1039) for it. (2) I went through the code you mentioned. I think it won't cause problem but it might miss protecting certain images (very rare case like calling gBS->LoadImage() from SMM driver entry point). I'll consult more relevant experts before filing a bug. (3) I'll update the commit message to reflect this for sure. Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, July 19, 2018 10:46 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] UefiCpuPkg/CpuDxe: fix incorrect check of SMM > mode > > On 07/18/18 04:35, Wang, Jian J wrote: > > Hi Laszlo, > > > > > > Regards, > > Jian > > > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:ler...@redhat.com] > >> Sent: Tuesday, July 17, 2018 10:37 PM > >> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > >> Cc: Dong, Eric <eric.d...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; > >> Zeng, Star <star.z...@intel.com> > >> 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/0c09afa07dd0434d9e2a0c6aeb0483103bb55...@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 <eric.d...@intel.com> > >>> Cc: Laszlo Ersek <ler...@redhat.com> > >>> Cc: Jiewen Yao <jiewen....@intel.com> > >>> Cc: Star Zeng <star.z...@intel.com> > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>> Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > >>> --- > >>> 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. > > (1) Please file a TianoCore BZ about fixing the indiscriminate > X64.PageTableBase access in IsInSmm(). > > The argument you make about GetCurrentPagingContext() is not valid, in > my opinion. It boils down to, "yeah, we're possibly accessing a random > gap in the IA32 structure, but hey that's no problem, we zero it out > anyway". This is very bad programming practice, not to mention the > extremely confusing source code reference to "X64" when we may be > running on IA32 in fact. I see some references in the code to > "Ia32.PageTableBase" as well, so there must be some way to tell apart > the cases. At the minimum, the PAGE_TABLE_LIB_PAGING_CONTEXT_* > structures should be packed. > > - Therefore, please file a TianoCore BZ about cleaning this up. > > - In addition, please reference that TianoCore BZ in the commit message. > > (Normally I would request a new version with this update, but I realize > this patch may be urgent for you. So let's add a forward-reference to > the commit message, in the form of a new TianoCore BZ.) > > (2) We have another instance of IsInSmm(), in > "MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c". Please evaluate > whether > the same issue affects it. If so, can you please file another TianoCore > BZ about fixing that? > > From my side, it is not necessary to reference *that* TianoCore BZ in > the commit message. > > (3) Please correct the commit hash reference in the commit message, from > d106cf71eabaacff63c14626a4a87346b93074dd to 2a1408d1d739. > > > With (1) and (3) addressed, you can add my: > > Regression-tested-by: Laszlo Ersek <ler...@redhat.com> > > > Also, sorry about the delay. > > Thanks, > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
