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

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

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