Re: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced BaseStackCheckLib

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

On Aug 4, 2014, at 8:30 AM, Olivier Martin <oli...@ar...> wrote:

> Recommended value does not mean unique value. Anyway, no one has commented whether this value should be kept in the code or be replaced by a PCD. So, I will keep the original code.
> I will send a new patchset in the next couple of minutes to define this class instance as NULL instead of BASE.
>  

The recommendation is to use a constant like the one in the code, or a random number per boot.  So if there was a PCD it would be to use the constant vs. a random number per boot. I don’t think we have a BaseLib compatible way to get a random number?

The per boot random number makes it harder for attacking code to figure out where the “canary” is so it can be defeated. It is like ASLR, and a constant PCD value is not going to help with that. 

Thanks,

Andrew Fish

> Olivier
>  
> From: Gao, Liming [mailto:lim...@in...] 
> Sent: 09 July 2014 03:42
> To: Olivier Martin; 'Andrew Fish'
> Cc: Kinney, Michael D; edk...@li...; edk...@li...
> Subject: RE: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced BaseStackCheckLib
>  
> Martin:
>   Per Andrew comment, this value is the recommended constant. If so, we don’t need to  add one PCD for it unless someone has the strong request to configure it.
>  
> Thanks
> Liming
> From: Olivier Martin [mailto:oli...@ar...] 
> Sent: Tuesday, July 08, 2014 9:26 PM
> To: 'Andrew Fish'; Gao, Liming
> Cc: Kinney, Michael D; edk...@li...; edk...@li...
> Subject: RE: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced BaseStackCheckLib
>  
> Actually, I was thinking to replace this canary value by a FixedPcd but I do not remember why I have not done it. I might have just forgot it.
> Olivier
>  
> From: Andrew Fish [mailto:af...@ap...] 
> Sent: 08 July 2014 02:14
> To: Gao, Liming
> Cc: Olivier Martin; Mike Kinney; edk...@li...; edk...@li...
> Subject: Re: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced BaseStackCheckLib
>  
>  
> On Jul 7, 2014, at 5:40 PM, Gao, Liming <lim...@in...> wrote:
>  
> 
> Martin:
>  What is 0x0AFF for? Is it an address or a value?
>  
> This value is the recommended constant if you can not generate a real random number for the “canary” value. 
>  
> It has NULL (will terminate strings), LF, and -1. http://wiki.osdev.org/GCC_Stack_Smashing_Protector. So it helps contains read based overruns. 
>  
> __stack_chk_guard is the “canary” placed on the stack by the compiler. __stack_check_fail() is called if the “canary” has been over written. These are both compiler intrinsics.
>  
> Thanks,
>  
> Andrew Fish
>  
> ~/work/Compiler>cat stack.c
>  
> void
> test (int i, char v)
> {
>   char test[0x100];
>  
>   test[i] = v;
>   return; 
> }
>  
> ~/work/Compiler>clang -S stack.c
> ~/work/Compiler>cat stack.S
>               .section    __TEXT,__text,regular,pure_instructions
>               .globl       _test
>               .align        4, 0x90
> _test:                                  ## @test
>               .cfi_startproc
> ## BB#0:
>               pushq      %rbp
> Ltmp2:
>               .cfi_def_cfa_offset 16
> Ltmp3:
>               .cfi_offset %rbp, -16
>               movq       %rsp, %rbp
> Ltmp4:
>               .cfi_def_cfa_register %rbp
>               subq        $272, %rsp              ## imm = 0x110
>               movb       %sil, %al
>               movq       ___stack_chk_guard@GOTPCREL(%rip), %rcx
>               movq       (%rcx), %rcx
>               movq       %rcx, -8(%rbp)
>               movq       ___stack_chk_guard@GOTPCREL(%rip), %rcx
>               movl        %edi, -12(%rbp)
>               movb       %al, -13(%rbp)
>               movb       -13(%rbp), %al
>               movslq    -12(%rbp), %rdx
>               movb       %al, -272(%rbp,%rdx)
>               movq       (%rcx), %rcx
>               movq       -8(%rbp), %rdx
>               cmpq       %rdx, %rcx
>               jne           LBB0_2
> ## BB#1:                                ## %SP_return
>               addq        $272, %rsp              ## imm = 0x110
>               popq        %rbp
>               ret
> LBB0_2:                                 ## %CallStackCheckFailBlk
>               callq         ___stack_chk_fail
>               .cfi_endproc
>  
>  
> .subsections_via_symbols
>  
>  
> 
> +/// "canary" value that is inserted by the compiler into the stack frame.
> +VOID *__stack_chk_guard = (VOID*)0x0AFF;
> 
>  And, this library instance is used as NULL class instance. Its library class should be NULL. 
> 
> Thanks
> Liming
> -----Original Message-----
> From: Olivier Martin [mailto:oli...@ar...] 
> Sent: Monday, July 07, 2014 11:44 PM
> To: Kinney, Michael D; edk...@li...
> Cc: and...@ap...; edk...@li...
> Subject: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced BaseStackCheckLib
> 
> This library only support GCC and XCode for now.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Andrew Fish <and...@ap...>
> Signed-off-by: Olivier Martin <oli...@ar...
> 
> Change-Id: Ib51239b7d315637fd22c34a9a78a7a830f2ffdc7
> ---
> .../Library/BaseStackCheckLib/BaseStackCheckGcc.c  | 62 ++++++++++++++++++++++
> .../BaseStackCheckLib/BaseStackCheckLib.inf        | 41 ++++++++++++++
> 2 files changed, 103 insertions(+)
> create mode 100644 MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> create mode 100644 MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> 
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> new file mode 100644
> index 0000000..9ec76d8
> --- /dev/null
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c
> @@ -0,0 +1,62 @@
> +/** @file
> + Base Stack Check library for GCC/clang.
> +
> + Use -fstack-protector-all compiler flag to make the compiler insert 
> + the __stack_chk_guard "canary" value into the stack and check the 
> + value prior to exiting the function. If the "canary" is overwritten 
> + __stack_chk_fail() is called. This is GCC specific code.
> +
> + Copyright (c) 2012, Apple Inc. 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.
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +
> +VOID
> +__stack_chk_fail (
> + VOID
> + );
> +
> +
> +/// "canary" value that is inserted by the compiler into the stack frame.
> +VOID *__stack_chk_guard = (VOID*)0x0AFF;
> +
> +// If ASLR was enabled we could use
> +//void (*__stack_chk_guard)(void) = __stack_chk_fail;
> +
> +/**
> + Error path for compiler generated stack "canary" value check code. If 
> +the  stack canary has been overwritten this function gets called on 
> +exit of the  function.
> +**/
> +VOID
> +__stack_chk_fail (
> + VOID
> + )
> +{
> +  UINT8 DebugPropertyMask;
> +
> +  DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow in function 
> + %a.\n", __builtin_return_address(0)));
> +
> +  //
> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even 
> +if
> +  // BaseDebugLibNull is in use.
> +  //
> +  DebugPropertyMask = PcdGet8 (PcdDebugPropertyMask);
> +  if ((DebugPropertyMask & DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> +    CpuBreakpoint ();
> +  } else if ((DebugPropertyMask & DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> +   CpuDeadLoop ();
> +  }
> +}
> +
> diff --git a/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> new file mode 100644
> index 0000000..4e2285d
> --- /dev/null
> +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> @@ -0,0 +1,41 @@
> +## @file
> +#  Stack Check Library
> +#
> +#  Copyright (c) 2014, ARM Ltd. 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.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = BaseStackCheckLib
> +  FILE_GUID                      = 5f6579f7-b648-4fdb-9f19-4c17e27e8eff
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = BaseStackCheckLib
> +
> +
> +#
> +#  VALID_ARCHITECTURES           = ARM AARCH64
> +#
> +
> +[Sources]
> +  BaseStackCheckGcc.c | GCC
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +
> +[Pcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask
> +
> --
> 1.8.5
> 
> 
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
> _______________________________________________
> edk2-buildtools-devel mailing list
> edk...@li...
> https://lists.sourceforge.net/lists/listinfo/edk2-buildtools-devel
> 
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
> _______________________________________________
> edk2-buildtools-devel mailing list
> edk...@li...
> https://lists.sourceforge.net/lists/listinfo/edk2-buildtools-devel