On Aug 4, 2014, at 8:50 AM, Olivier Martin <olivier.mar...@arm.com> wrote:
> 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.
>
I guess the PCD could be useful for debugging a specific issue as well.
As long as the reasoning behind the default value for the “canary” is
documented in the PCD default I don’t see an issue using a FixedAtBuildPcd.
Thanks,
Andrew Fish
> Olivier
>
> From: Andrew Fish [mailto:af...@apple.com]
> Sent: 04 August 2014 16:44
> To: Olivier Martin
> Cc: Gao, Liming; Mike Kinney; edk2-buildtools-de...@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.mar...@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....@intel.com]
> Sent: 09 July 2014 03:42
> To: Olivier Martin; 'Andrew Fish'
> Cc: Kinney, Michael D; edk2-buildtools-de...@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.mar...@arm.com]
> Sent: Tuesday, July 08, 2014 9:26 PM
> To: 'Andrew Fish'; Gao, Liming
> Cc: Kinney, Michael D; edk2-buildtools-de...@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:af...@apple.com]
> Sent: 08 July 2014 02:14
> To: Gao, Liming
> Cc: Olivier Martin; Mike Kinney; edk2-buildtools-de...@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 <liming....@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. 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.mar...@arm.com]
> Sent: Monday, July 07, 2014 11:44 PM
> To: Kinney, Michael D; edk2-buildtools-de...@lists.sourceforge.net
> Cc: andrew.f...@apple.com; 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 <andrew.f...@apple.com>
> Signed-off-by: Olivier Martin <olivier.mar...@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
>
> 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
> edk2-buildtools-de...@lists.sourceforge.net
> 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
> edk2-buildtools-de...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-buildtools-devel
------------------------------------------------------------------------------
Infragistics Professional
Build stunning WinForms apps today!
Reboot your WinForms applications with our WinForms controls.
Build a bridge from your legacy apps to the future.
http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
