Re: [PATCH 2] UefiCpuPkg: restore strict page attributes via #DB in nonstop mode only

[Laszlo Ersek <lersek@redhat.com>]

On 03/01/19 04:26, Wang, Jian J wrote:
> Thanks. To catch cold freeze, pushed earlier (2a93cccc24cfca12c66f13a41d52fb0a82fb924e)

This is definitely a bugfix, so it is eligible for pushing.

Thanks
Laszlo

>> -----Original Message-----
>> From: Dong, Eric
>> Sent: Friday, March 01, 2019 9:55 AM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH 2] UefiCpuPkg: restore strict page attributes via #DB
>> in nonstop mode only
>>
>> Reviewed-by: Eric Dong <eric.dong@intel.com>
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Jian J Wang
>>> Sent: Friday, March 1, 2019 8:58 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong,
>>> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
>>> Subject: [edk2] [PATCH 2] UefiCpuPkg: restore strict page attributes via #DB
>>> in nonstop mode only
>>>
>>>> v2: Per Laszlo's comments, repack origianl two patches into one with
>>>>     title changed and relevant commits added
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1576
>>>
>>> The root cause of this issue is that non-stop mode of Heap Guard and NULL
>>> Detection set TF bit (single-step) in EFLAG unconditionally in the common
>>> handler in CpuExceptionLib.
>>>
>>> If PcdCpuSmmStaticPageTable is FALSE, the SMM will only create page table
>>> for memory below 4G. If SMM tries to access memory beyond 4G, a page
>>> fault exception will be triggered and the memory to access will be added to
>>> page table so that SMM code can continue the access.
>>>
>>> Because of above issue, the TF bit is set after the page fault is handled and
>>> then fall into another DEBUG exception. Since non-stop mode of Heap Guard
>>> and NULL Detection are not enabled, no special DEBUG exception handler is
>>> registered. The default handler just prints exception context and go into
>>> dead loop.
>>>
>>> Actually EFLAGS can be changed in any standard exception handler.
>>> There's no need to do single-step setup in assembly code. So the fix is to
>>> move the logic to C code part of page fault exception handler so that we can
>>> fully validate the configuration and prevent TF bit from being set
>>> unexpectedly.
>>>
>>> Fixes: dcc026217fdc363f55c217039fc43d344f69fed6
>>>        16b918bbaf51211a32ae04d9d8a5ba6ccca25a6a
>>> Test:
>>>  - Pass special test of accessing memory beyond 4G in SMM mode
>>>  - Boot to OS with Qemu emulator platform (Fedora27, Ubuntu18.04,
>>>    Windows7, Windows10)
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ruiyu Ni <ruiyu.ni@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>
>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  UefiCpuPkg/CpuDxe/CpuPageTable.c                      | 11 ++++++++++-
>>>  .../Ia32/ExceptionHandlerAsm.nasm                     |  7 -------
>>>  .../X64/ExceptionHandlerAsm.nasm                      |  4 ----
>>>  3 files changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> index 4bee8c7772..812537417d 100644
>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> @@ -1300,7 +1300,16 @@ PageFaultExceptionHandler (
>>>    // Display ExceptionType, CPU information and Image information
>>>    //
>>>    DumpCpuContext (ExceptionType, SystemContext);
>>> -  if (!NonStopMode) {
>>> +  if (NonStopMode) {
>>> +    //
>>> +    // Set TF in EFLAGS
>>> +    //
>>> +    if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
>>> +      SystemContext.SystemContextIa32->Eflags |= (UINT32)BIT8;
>>> +    } else {
>>> +      SystemContext.SystemContextX64->Rflags |= (UINT64)BIT8;
>>> +    }
>>> +  } else {
>>>      CpuDeadLoop ();
>>>    }
>>>  }
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.
>>> nasm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.
>>> nasm
>>> index 6fcf5fb23f..45d6474091 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.
>>> nasm
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm
>>> +++ .nasm
>>> @@ -383,13 +383,6 @@ ErrorCodeAndVectorOnStack:
>>>      pop     dword [ebp - 4]
>>>      mov     esp, ebp
>>>      pop     ebp
>>> -
>>> -; Enable TF bit after page fault handler runs
>>> -    cmp     dword [esp], 14       ; #PF?
>>> -    jne     .5
>>> -    bts     dword [esp + 16], 8   ; EFLAGS
>>> -
>>> -.5:
>>>      add     esp, 8
>>>      cmp     dword [esp - 16], 0   ; check
>>> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
>>>      jz      DoReturn
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
>>> asm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
>>> asm
>>> index f842af2336..7b97810d10 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
>>> asm
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.
>>> +++ nasm
>>> @@ -336,10 +336,6 @@ HasErrorCode:
>>>      pop     r15
>>>
>>>      mov     rsp, rbp
>>> -    cmp     qword [rbp + 8], 14 ; #PF?
>>> -    jne     .1
>>> -    bts     qword [rsp + 40], 8 ; RFLAGS.TF
>>> -.1:
>>>      pop     rbp
>>>      add     rsp, 16
>>>      cmp     qword [rsp - 32], 0  ; check
>>> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
>>> --
>>> 2.17.1.windows.2
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel