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

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

[Jian J Wang]

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. 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).

This is an issue caused by check-in at

  d106cf71eabaacff63c14626a4a87346b93074dd

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());
 }
 
 /**
-- 
2.16.2.windows.1

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

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Wang, Jian J
> Sent: Friday, July 13, 2018 1:54 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH] UefiCpuPkg/CpuDxe: fix incorrect check of SMM mode
> 
> 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. 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).
> 
> This is an issue caused by check-in at
> 
>   d106cf71eabaacff63c14626a4a87346b93074dd
> 
> 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());
>  }
> 
>  /**
> --
> 2.16.2.windows.1

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

[Laszlo Ersek]

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.ccr.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?

> 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?

> 
> 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).


How did you encounter / find this issue?

> 
> 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.


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.)

Thanks,
Laszlo

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

[Wang, Jian J]

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

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

[Laszlo Ersek]

On 07/18/18 04:35, Wang, Jian J wrote:
> 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.

(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 <lersek@redhat.com>


Also, sorry about the delay.

Thanks,
Laszlo

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

[Wang, Jian J]

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:lersek@redhat.com]
> Sent: Thursday, July 19, 2018 10:46 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@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: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.
> 
> (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 <lersek@redhat.com>
> 
> 
> Also, sorry about the delay.
> 
> Thanks,
> Laszlo

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

[Wang, Jian J]

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

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

[Laszlo Ersek]

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 <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