Re: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Laszlo,

Do you have more comments? Or can you give a r-b?

Regards,
Jian

> -----Original Message-----
> From: Wang, Jian J
> Sent: Wednesday, July 18, 2018 10:36 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Zeng, Star <star.zeng@intel.com>
> 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 <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> > Zeng, Star <star.zeng@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/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 <eric.dong@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang <jian.j.wang@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.
> 
> > Thanks,
> > Laszlo