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

[Laszlo Ersek <lersek@redhat.com>]

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