diff mbox

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

Message ID 53dfabfa.c7f72a0a.4cbd.ffffed76SMTPIN_ADDED_BROKEN@mx.google.com
State New
Headers show

Commit Message

Olivier Martin Aug. 4, 2014, 3:50 p.m. UTC
The benefit of the FixedPcd was to be an easy alternative between the
recommended constant value and the random number.

A Firmware vendor might want to use a different canary value as the open
source project.

 

Olivier

 

From: Andrew Fish [mailto:afish@apple.com] 
Sent: 04 August 2014 16:44
To: Olivier Martin
Cc: Gao, Liming; Mike Kinney; edk2-buildtools-devel@lists.sourceforge.net;
edk2-devel@lists.sourceforge.net
Subject: Re: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced
BaseStackCheckLib

 

 

On Aug 4, 2014, at 8:30 AM, Olivier Martin <olivier.martin@arm.com> 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:liming.gao@intel.com] 
Sent: 09 July 2014 03:42
To: Olivier Martin; 'Andrew Fish'
Cc: Kinney, Michael D; edk2-buildtools-devel@lists.sourceforge.net;
edk2-devel@lists.sourceforge.net
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:olivier.martin@arm.com>
mailto:olivier.martin@arm.com] 
Sent: Tuesday, July 08, 2014 9:26 PM
To: 'Andrew Fish'; Gao, Liming
Cc: Kinney, Michael D;  <mailto:edk2-buildtools-devel@lists.sourceforge.net>
edk2-buildtools-devel@lists.sourceforge.net;
<mailto:edk2-devel@lists.sourceforge.net> edk2-devel@lists.sourceforge.net
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:afish@apple.com> mailto:afish@apple.com] 
Sent: 08 July 2014 02:14
To: Gao, Liming
Cc: Olivier Martin; Mike Kinney;
<mailto:edk2-buildtools-devel@lists.sourceforge.net>
edk2-buildtools-devel@lists.sourceforge.net;
<mailto:edk2-devel@lists.sourceforge.net> edk2-devel@lists.sourceforge.net
Subject: Re: [edk2-buildtools] [PATCH 2/3] MdePkg: Introduced
BaseStackCheckLib

 

 

On Jul 7, 2014, at 5:40 PM, Gao, Liming < <mailto:liming.gao@intel.com>
liming.gao@intel.com> 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>
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:olivier.martin@arm.com>
mailto:olivier.martin@arm.com] 
Sent: Monday, July 07, 2014 11:44 PM
To: Kinney, Michael D;  <mailto:edk2-buildtools-devel@lists.sourceforge.net>
edk2-buildtools-devel@lists.sourceforge.net
Cc:  <mailto:andrew.fish@apple.com> andrew.fish@apple.com;
<mailto:edk2-devel@lists.sourceforge.net> edk2-devel@lists.sourceforge.net
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 < <mailto:andrew.fish@apple.com>
andrew.fish@apple.com>
Signed-off-by: Olivier Martin < <mailto:olivier.martin@arm.com>
olivier.martin@arm.com

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

+
--
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> http://p.sf.net/sfu/Bonitasoft
diff mbox

Patch

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