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

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

Thanks. To catch cold freeze, pushed earlier (2a93cccc24cfca12c66f13a41d52fb0a82fb924e)

Regards,
Jian


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