Re: [PATCH] MdePkg/BaseStackCheckLib: add MSFT toolchain support

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

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1239

Regards,
Jian


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> Sent: Tuesday, October 16, 2018 8:55 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Andrew Fish <afish@apple.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH] MdePkg/BaseStackCheckLib: add MSFT toolchain
> support
> 
> This patch adds stack check support for MSFT toolchain, with
> compiler option /GS and /RTCs. This functionality is similar
> to the original ones supported by GCC toolchain.
> 
> Usage example:
> This is a NULL library instance. Add it under a [LibraryClasses]
> section in dsc file to let it be built into all modules employed
> in a platform.
> 
>     [LibraryClasses]
>       NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> 
> Please note all not modules can be built against this library. Most
> of them are SEC type of modules, such as
> 
>     OvmfPkg/ResetVector/ResetVector.inf
> 
> In this case, this library should not be added to a common
> [LibraryClasses] section but to specific ones, like
> [LibraryClasses.common.PEI_CORE/PEIM/...].
> 
> In addition, /GS and/or /RTCs should be added to compiler command line.
> This can be done by adding something like below under [BuildOptions]
> section in dsc file.
> 
>     [BuildOptions]
>       MSFT:DEBUG_*_*_CC_FLAGS = /GS /GL-
>       MSFT:DEBUG_*_*_CC_FLAGS = /RTCs /Od
> 
> Note: /GL- is required for /GS, and /Od is required for /RTCs.
> Note: The flash layout might be needed to update to accommodate larger
>       image size due to /Od is enforced.
> 
> Pass tests:
> a. Overwrite a local buffer variable (in a 32-bit and 64-bit driver)and
>    check if it's caught by new code (on both real platform and virtual
>    platform)
> b. Boot Windows 10 and Ubuntu 18.04 on real platform with this
>    lib built-in
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  .../BaseStackCheckLib/BaseStackCheckLib.inf        |  11 +-
>  .../Library/BaseStackCheckLib/BaseStackCheckMsft.c | 221
> +++++++++++++++++++++
>  .../Library/BaseStackCheckLib/BaseStackCheckNull.c |  15 --
>  .../BaseStackCheckLib/Ia32/StackCheckStubAsm.nasm  |  76 +++++++
>  .../BaseStackCheckLib/X64/StackCheckStubAsm.nasm   |  54 +++++
>  5 files changed, 360 insertions(+), 17 deletions(-)
>  create mode 100644
> MdePkg/Library/BaseStackCheckLib/BaseStackCheckMsft.c
>  delete mode 100644 MdePkg/Library/BaseStackCheckLib/BaseStackCheckNull.c
>  create mode 100644
> MdePkg/Library/BaseStackCheckLib/Ia32/StackCheckStubAsm.nasm
>  create mode 100644
> MdePkg/Library/BaseStackCheckLib/X64/StackCheckStubAsm.nasm
> 
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> index e280651b11..1c9e6710c6 100644
> --- a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> @@ -4,6 +4,7 @@
>  #  Stack Check Library
>  #
>  #  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
> +#  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD
> License
> @@ -26,13 +27,19 @@
> 
> 
>  #
> -#  VALID_ARCHITECTURES           = ARM AARCH64
> +#  VALID_ARCHITECTURES           = ARM AARCH64 IA32 X64
>  #
> 
>  [Sources]
>    BaseStackCheckGcc.c  | GCC
>    BaseStackCheckGcc.c  | RVCT
> -  BaseStackCheckNull.c | MSFT
> +  BaseStackCheckMsft.c | MSFT
> +
> +[Sources.IA32]
> +  Ia32/StackCheckStubAsm.nasm | MSFT
> +
> +[Sources.X64]
> +  X64/StackCheckStubAsm.nasm | MSFT
> 
>  [Packages]
>    MdePkg/MdePkg.dec
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckMsft.c
> b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckMsft.c
> new file mode 100644
> index 0000000000..951154f0cd
> --- /dev/null
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckMsft.c
> @@ -0,0 +1,221 @@
> +/** @file
> + Base Stack Check library for MSFT toolchains compiler options: /GS, RTCs.
> +
> +Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> +This program and the accompanying materials are licensed and made available
> under
> +the terms and conditions of the BSD License that accompanies this distribution.
> +The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> +
> +**/
> +
> +#include <Base.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +
> +//
> +// cookie value that is inserted by the MSFT compiler into the stack frame.
> +//
> +extern UINTN __security_cookie;
> +
> +//
> +// Data structure used by MSFT compiler to record local variable information.
> +//
> +
> +typedef struct _RTC_vardesc {
> +  int   Addr;
> +  int   Size;
> +  char  *Name;
> +} _RTC_vardesc;
> +
> +typedef struct _RTC_framedesc {
> +  int           VarCount;
> +  _RTC_vardesc  *Variables;
> +} _RTC_framedesc;
> +
> +#define RTC_STACK_CHECK_COOKIE  0xCCCCCCCC
> +
> +/**
> +  Function called upon unexpected stack pointer change.
> +
> +  @param Ip       Instruction address where the check happened.
> +
> +**/
> +VOID
> +__cdecl
> +_RTC_Failure (
> +  VOID    *Ip
> +  )
> +{
> +  DEBUG ((DEBUG_ERROR, "\nSTACK FAULT: Suspicious stack pointer
> (IP:%p).\n\n", Ip));
> +
> +  //
> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even if
> +  // BaseDebugLibNull is in use.
> +  //
> +  if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> +    CpuBreakpoint ();
> +  } else {
> +    //
> +    // Usually the boot should stop here if check failure. Due to the fact
> +    // that the normal Stack Switch happened in boot will also fail the stack
> +    // pointer check. So no dead loop here.
> +    //
> +  }
> +  return;
> +}
> +
> +/**
> +  Function reporting stack buffer overlow.
> +
> +  @param Name     Local varible name.
> +  @param Ip       Instruction address where the check happened.
> +
> +**/
> +STATIC
> +VOID
> +_RTC_StackFailure (
> +  CHAR8   *Name,
> +  VOID    *Ip
> +  )
> +{
> +  DEBUG ((DEBUG_ERROR, "\nSTACK FAULT: Local variable '%a' overflow
> (IP:%p).\n\n", Name, Ip));
> +
> +  //
> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even if
> +  // BaseDebugLibNull is in use.
> +  //
> +  if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> +    CpuBreakpoint ();
> +  } else if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> +   CpuDeadLoop ();
> +  }
> +  return ;
> +}
> +
> +/**
> +  Function called upon stack buffer overflow. (/RTCs)
> +
> +  @param _Esp     Stack frame pointer.
> +  @param _Fd      Pointer to local variable information.
> +
> +**/
> +VOID
> +__fastcall
> +_RTC_CheckStackVars (
> +  VOID            *_Esp,
> +  _RTC_framedesc  *_Fd
> +  )
> +{
> +  INTN      Index;
> +  UINT8     *Addr;
> +
> +  for (Index = 0; Index < _Fd->VarCount; Index++) {
> +    Addr = (UINT8 *)_Esp + _Fd->Variables[Index].Addr - sizeof(UINT32);
> +    if (*(UINT32 *)Addr != RTC_STACK_CHECK_COOKIE) {
> +      _RTC_StackFailure (_Fd->Variables[Index].Name, RETURN_ADDRESS(0));
> +    }
> +
> +    Addr = (UINT8 *)_Esp + _Fd->Variables[Index].Addr + _Fd-
> >Variables[Index].Size;
> +    if (*(UINT32 *)Addr != RTC_STACK_CHECK_COOKIE) {
> +      _RTC_StackFailure (_Fd->Variables[Index].Name, RETURN_ADDRESS(0));
> +    }
> +  }
> +}
> +
> +/**
> +  Function required by linker but not implemented by firmware image loader.
> (/RTCs)
> +
> +**/
> +VOID
> +__cdecl
> +_RTC_Shutdown (
> +  VOID
> +  )
> +{
> +  return;
> +}
> +
> +/**
> +  Function required by linker but not implemented by firmware image loader.
> (/RTCs)
> +
> +**/
> +VOID
> +__cdecl
> +_RTC_InitBase (
> +  VOID
> +  )
> +{
> +  return;
> +}
> +
> +
> +/**
> +  Function called upon stack frame overflow detected. (/GS)
> +
> +  @param StackCookie    Actual cookie value got from stack boundary.
> +  @param Ip             Instruction address where the check happened.
> +
> +**/
> +NORETURN
> +VOID
> +__cdecl
> +__report_gsfailure (
> +  UINTN     StackCookie,
> +  VOID      *Ip
> +  )
> +{
> +  DEBUG ((DEBUG_ERROR, "\nSTACK FAULT: Stack overflow check failed in
> cookie checker (IP:%p).\n\n", Ip));
> +
> +  //
> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even if
> +  // BaseDebugLibNull is in use.
> +  //
> +  if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> +    CpuBreakpoint ();
> +  } else if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> +   CpuDeadLoop ();
> +  }
> +}
> +
> +/**
> +  Function called upon failure at local array range check . (/GS)
> +
> +**/
> +NORETURN
> +VOID
> +__cdecl
> +__report_rangecheckfailure (
> +  VOID
> +  )
> +{
> +  DEBUG((DEBUG_ERROR, "\nSTACK FAULT: Range check check failed in cookie
> checker.\n\n"));
> +
> +  //
> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even if
> +  // BaseDebugLibNull is in use.
> +  //
> +  if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> +    CpuBreakpoint ();
> +  } else if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> +   CpuDeadLoop ();
> +  }
> +}
> +
> +/**
> +  Function required by linker but not implemented by firmware image loader.
> (/GS)
> +
> +**/
> +VOID
> +__GSHandlerCheck (
> +  VOID
> +  )
> +{
> +  return;
> +}
> +
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckNull.c
> b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckNull.c
> deleted file mode 100644
> index 7c27c73e23..0000000000
> --- a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckNull.c
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/** @file
> - This file is purely empty as a work around for BaseStackCheck to pass MSVC
> build.
> -
> - Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> - This program and the accompanying materials
> - are licensed and made available under the terms and conditions of the BSD
> License
> - which accompanies this distribution.  The full text of the license may be found
> at
> - http://opensource.org/licenses/bsd-license.php.
> -
> - THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> - WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> -
> -**/
> -
> -extern int __BaseStackCheckNull;
> diff --git a/MdePkg/Library/BaseStackCheckLib/Ia32/StackCheckStubAsm.nasm
> b/MdePkg/Library/BaseStackCheckLib/Ia32/StackCheckStubAsm.nasm
> new file mode 100644
> index 0000000000..d3c8d32161
> --- /dev/null
> +++ b/MdePkg/Library/BaseStackCheckLib/Ia32/StackCheckStubAsm.nasm
> @@ -0,0 +1,76 @@
> +;------------------------------------------------------------------------------ ;
> +; Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> +; This program and the accompanying materials
> +; are licensed and made available under the terms and conditions of the BSD
> License
> +; which accompanies this distribution.  The full text of the license may be found
> at
> +; http://opensource.org/licenses/bsd-license.php.
> +;
> +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +;
> +; Module Name:
> +;
> +;   StackCheckStubAsm.nasm
> +;
> +; Abstract:
> +;
> +;   Stub globals and functions for compiler options /GS, /RTCs
> +;
> +; Notes:
> +;
> +;------------------------------------------------------------------------------
> +
> +;
> +; __declspec(noreturn) void __cdecl __report_gsfailure(UINTN cookie, void *ip);
> +;
> +extern ___report_gsfailure
> +;
> +; void __cdecl _RTC_Failure (void *Ip);
> +;
> +extern __RTC_Failure
> +
> +SECTION .data
> +
> +;
> +; UINTN __security_cookie;
> +;
> +global ___security_cookie
> +___security_cookie:
> +    DW     987974FAh
> +
> +SECTION .text
> +
> +;
> +; void __fastcall __security_check_cookie(UINTN cookie)
> +;
> +;   Note: __fastcall calling convention uses ecx/edx to pass first two parameters
> +;
> +global @__security_check_cookie@4
> +@__security_check_cookie@4:
> +    push        ebp
> +    mov         ebp, esp
> +    cmp         ecx, [___security_cookie]
> +    je          .1
> +    push        dword [ebp] ; pass return address as the second parameter
> +    push        ecx         ; cookie value in stack is the first parameter
> +    call        ___report_gsfailure
> +.1:
> +    mov         esp, ebp
> +    pop         ebp
> +    ret
> +
> +;
> +; void __declspec(naked) __cdecl _RTC_CheckEsp(void)
> +;
> +global __RTC_CheckEsp
> +__RTC_CheckEsp:
> +    push        ebp
> +    mov         ebp, esp
> +    je         .1
> +    push       dword [ebp]  ; pass return address to __RTC_Failure
> +    call        __RTC_Failure
> +.1:
> +    mov         esp, ebp
> +    pop         ebp
> +    ret
> +
> diff --git a/MdePkg/Library/BaseStackCheckLib/X64/StackCheckStubAsm.nasm
> b/MdePkg/Library/BaseStackCheckLib/X64/StackCheckStubAsm.nasm
> new file mode 100644
> index 0000000000..1c8601f09c
> --- /dev/null
> +++ b/MdePkg/Library/BaseStackCheckLib/X64/StackCheckStubAsm.nasm
> @@ -0,0 +1,54 @@
> +;------------------------------------------------------------------------------ ;
> +; Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> +; This program and the accompanying materials
> +; are licensed and made available under the terms and conditions of the BSD
> License
> +; which accompanies this distribution.  The full text of the license may be found
> at
> +; http://opensource.org/licenses/bsd-license.php.
> +;
> +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +;
> +; Module Name:
> +;
> +;   StackCheckStubAsm.nasm
> +;
> +; Abstract:
> +;
> +;   Stub globals and functions for compiler options /GS, /RTCs
> +;
> +; Notes:
> +;
> +;------------------------------------------------------------------------------
> +
> +;
> +; __declspec(noreturn) void __cdecl __report_gsfailure(UINTN cookie, void *ip);
> +;
> +extern __report_gsfailure
> +
> +DEFAULT REL
> +
> +SECTION .data
> +
> +;
> +; UINTN __security_cookie;
> +;
> +global __security_cookie
> +__security_cookie:
> +    DQ     0CFE3FE6A3F5C5A88h
> +
> +SECTION .text
> +
> +;
> +; void __fastcall __security_check_cookie(UINTN cookie)
> +;
> +;   Note: __fastcall calling convention uses ecx/edx to pass first two parameters
> +;
> +global __security_check_cookie
> +__security_check_cookie:
> +    cmp         rcx, qword [__security_cookie]
> +    je          .1
> +    mov         rdx, [esp]  ; pass return address as the second parameter
> +    call        __report_gsfailure
> +.1
> +    ret
> +
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel