Hi Olivier & Liming, Please help to review this patch, thanks.
Refine MdePkg: U BaseStackCheckLib library code to follow edkii coding style. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Eric Dong <eric.d...@intel.com> Thanks, Eric -----Original Message----- From: Olivier Martin [mailto:olivier.mar...@arm.com] Sent: Thursday, August 21, 2014 5:27 PM To: Gao, Liming; edk2-devel@lists.sourceforge.net; Kinney, Michael D Subject: Re: [edk2] [PATCH v3 1/3] MdePkg: Introduced BaseStackCheckLib Sorry, I committed the v3 patch without taking attention of its content. The FixedPcdGet() works well in the NULL library because the library is linked to a module. So the FixedPcdGet behaves correctly in this case. But the library cannot be built by itself with FixedPcdGet(). I think it was better to keep the PCD. But you are the maintainer. So, I have just pushed SVN rev15866 that makes the changes you suggested (that removes the PCD). Thanks, Olivier > -----Original Message----- > From: Gao, Liming [mailto:liming....@intel.com] > Sent: 21 August 2014 09:59 > To: Olivier Martin; edk2-devel@lists.sourceforge.net; Kinney, Michael > D > Subject: RE: [edk2] [PATCH v3 1/3] MdePkg: Introduced > BaseStackCheckLib > > Oliver: > I see your commit patch still includes FixedPcdGet() usage in > library. Does it pass build? If not, how about remove PCD first. > > And, another issue that new introduced PCD > gEfiMdePkgTokenSpaceGuid.PcdBaseStackCanary token number is same to > gEfiMdePkgTokenSpaceGuid.PcdValidateOrderedCollection. But, PCD token > number should be unique. So, its token number should be changed to > another value, such as 0x0000002b. > > Thanks > Liming > -----Original Message----- > From: Olivier Martin [mailto:olivier.mar...@arm.com] > Sent: Wednesday, August 20, 2014 11:29 PM > To: Gao, Liming; edk2-devel@lists.sourceforge.net; Kinney, Michael D > Subject: RE: [edk2] [PATCH v3 1/3] MdePkg: Introduced > BaseStackCheckLib > > Are you happy to add your "Reviewed-By" for this patch? > If yes do you want me to commit it? Or you are happy to do it? > > Thanks, > Olivier > > > -----Original Message----- > > From: Gao, Liming [mailto:liming....@intel.com] > > Sent: 20 August 2014 16:23 > > To: Olivier Martin; edk2-devel@lists.sourceforge.net; Kinney, > > Michael D > > Subject: RE: [edk2] [PATCH v3 1/3] MdePkg: Introduced > > BaseStackCheckLib > > > > Oliver: > > Sorry for late response. Library can't use FixedPcdGet(), because > it > > may be linked to the different drivers those may configure this PCD > > with the different value. That may be the reason you don't do it > > before. > > Because this value is the recommended constant, it should be fine > to > > hard code it first without PCD. > > > > Thanks > > Liming > > -----Original Message----- > > From: Olivier Martin [mailto:olivier.mar...@arm.com] > > Sent: Wednesday, August 20, 2014 5:47 PM > > To: edk2-devel@lists.sourceforge.net; Kinney, Michael D; Gao, Liming > > Subject: RE: [edk2] [PATCH v3 1/3] MdePkg: Introduced > > BaseStackCheckLib > > > > Any feedback? > > Thanks, > > Olivier > > > > > -----Original Message----- > > > From: Olivier Martin [mailto:olivier.mar...@arm.com] > > > Sent: 06 August 2014 15:49 > > > To: edk2-devel@lists.sourceforge.net; Kinney, Michael D; 'Gao, > > Liming' > > > Subject: Re: [edk2] [PATCH v3 1/3] MdePkg: Introduced > > > BaseStackCheckLib > > > > > > I addressed your comment 1. into my local patch. > > > > > > But for comment 2., I had initially added BaseStackLib to > MdePkg.dsc. > > > But it was not building as the FixedPcd was not defined: > > > ------- > > > > > > /home/olivier/tianocore/MdePkg/Library/BaseStackCheckLib/BaseStackChec > > > k > > > Gcc.c > > > :26:34: error: "_PCD_VALUE_PcdBaseStackCanary" undeclared here > > > (not > > in > > > a > > > function) > > > VOID *__stack_chk_guard = (VOID*)FixedPcdGet64 > > > (PcdBaseStackCanary); > > > ------- > > > > > > _PCD_VALUE_* are only generated when a binary module is built. And > > > there is no Module in MdePkg so adding > > > 'NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf' to > > > [LibraryClasses] would not help neither. > > > One solution would be to add > > > 'NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf' to > > > MdeModulePkg.dsc > > > > > > Which solution would you suggested for your comment 2.? > > > > > > Thanks, > > > Olivier > > > > > > > -----Original Message----- > > > > From: Gao, Liming [mailto:liming....@intel.com] > > > > Sent: 06 August 2014 04:00 > > > > To: edk2-devel@lists.sourceforge.net; Kinney, Michael D > > > > Subject: Re: [edk2] [PATCH v3 1/3] MdePkg: Introduced > > > BaseStackCheckLib > > > > > > > > Olivier: > > > > I have two minor comment. > > > > 1. The declaration of __stack_chk_fail() misses the function > > > comments. > > > > 2. Add it into Mdepkg.dsc [Components.ARM, Components.AARCH64] > > > section > > > > to verify its build. > > > > > > > > Other part is good to me. Reviewed-by: Gao, Liming > > > > <liming....@intel.com> > > > > > > > > Thanks > > > > Liming > > > > -----Original Message----- > > > > From: Olivier Martin [mailto:olivier.mar...@arm.com] > > > > Sent: Tuesday, August 05, 2014 5:48 PM > > > > To: Kinney, Michael D > > > > Cc: edk2-devel@lists.sourceforge.net > > > > Subject: [edk2] [PATCH v3 1/3] MdePkg: Introduced > > > > BaseStackCheckLib > > > > > > > > This library only support GCC, RVCT and XCode for now. > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > Signed-off-by: Andrew Fish <af...@apple.com> > > > > Signed-off-by: Olivier Martin <olivier.mar...@arm.com> > > > > --- > > > > .../Library/BaseStackCheckLib/BaseStackCheckGcc.c | 61 > > > > ++++++++++++++++++++++ > > > > .../BaseStackCheckLib/BaseStackCheckLib.inf | 42 > > > > +++++++++++++++ > > > > MdePkg/MdePkg.dec | 4 ++ > > > > 3 files changed, 107 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..4160aff > > > > --- /dev/null > > > > +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckGcc.c > > > > @@ -0,0 +1,61 @@ > > > > +/** @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*)FixedPcdGet64 > > > > +(PcdBaseStackCanary); > > > > + > > > > +// 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..3304284 > > > > --- /dev/null > > > > +++ b/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf > > > > @@ -0,0 +1,42 @@ > > > > +## @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 = NULL > > > > + > > > > + > > > > +# > > > > +# VALID_ARCHITECTURES = ARM AARCH64 > > > > +# > > > > + > > > > +[Sources] > > > > + BaseStackCheckGcc.c | GCC > > > > + BaseStackCheckGcc.c | RVCT > > > > + > > > > +[Packages] > > > > + MdePkg/MdePkg.dec > > > > + > > > > +[LibraryClasses] > > > > + BaseLib > > > > + DebugLib > > > > + > > > > +[FixedPcd] > > > > + gEfiMdePkgTokenSpaceGuid.PcdBaseStackCanary > > > > + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask > > > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index > > > > 4daf3e6..fbb7d2b 100644 > > > > --- a/MdePkg/MdePkg.dec > > > > +++ b/MdePkg/MdePkg.dec > > > > @@ -1544,6 +1544,10 @@ > > > > # The required memory space is decided by the value of > > > > PcdMaximumGuidedExtractHandler. > > > > > > > > > > > > > > gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x1000000 > > > | > > > > UINT64|0x30001015 > > > > > > > > + ## Canary value for the stack overflow protection. This PCD > can > > > > + be used by a firmware vendor # or for debugging purposes to > > > > + change > > > the > > > > recommended value. > > > > + > > > > gEfiMdePkgTokenSpaceGuid.PcdBaseStackCanary|0x0AFF|UINT64|0x0000002A > > > > + > > > > [PcdsFixedAtBuild.IPF] > > > > ## The base address of IO port space for IA64 arch > > > > > > > > > > > > > > gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000|UI > > > N > > > > T64|0x0000000f > > > > -- > > > > 1.8.5 > > > > > > > > > > > > ---------------------------------------------------------------- > > > > - > - > > > > - > > - > > > > - > > > -- > > > > ------- > > > > 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. > > > c > > > > lktrk > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/edk2-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. > > > c > > > > lktrk > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/edk2-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. > > > c > > > lktrk > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > > > > > > > > ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
BaseStackCheckGcc.c.patch
Description: BaseStackCheckGcc.c.patch
------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
_______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
