Add support for stack-protector

Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Usama Arif <usama.arif@arm.com>
Cc: Sam Protsenko <joe.skb7@gmail.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Philippe Reynes <philippe.reynes@softathome.com>
Cc: Eugeniu Rosca <roscaeugeniu@gmail.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>

Signed-off-by: Joel Peshkin <joel.peshkin@broadcom.com>

---

 Makefile             |  4 ++++
 common/Kconfig       | 15 +++++++++++++++
 common/Makefile      |  2 ++
 common/stackprot.c   | 17 +++++++++++++++++
 scripts/Makefile.spl |  6 ++++++
 5 files changed, 44 insertions(+)
 create mode 100644 common/stackprot.c

On 1/10/21 4:39 PM, Joel Peshkin wrote:
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Kever Yang <kever.yang@rock-chips.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Usama Arif <usama.arif@arm.com>
> Cc: Sam Protsenko <joe.skb7@gmail.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Philippe Reynes <philippe.reynes@softathome.com>
> Cc: Eugeniu Rosca <roscaeugeniu@gmail.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>
> Signed-off-by: Joel Peshkin <joel.peshkin@broadcom.com>
>
> ---
>
>   Makefile             |  4 ++++
>   common/Kconfig       | 15 +++++++++++++++
>   common/Makefile      |  2 ++
>   common/stackprot.c   | 17 +++++++++++++++++
>   scripts/Makefile.spl |  6 ++++++
>   5 files changed, 44 insertions(+)
>   create mode 100644 common/stackprot.c
>
> diff --git a/Makefile b/Makefile
> index 3ee4cc00dd..6e7a81ec7d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -677,7 +677,11 @@ else
>   KBUILD_CFLAGS	+= -O2
>   endif
>
> +ifeq ($(CONFIG_STACKPROTECTOR),y)
> +KBUILD_CFLAGS += $(call cc-option,-fstack-protector-strong)
> +else
>   KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
> +endif
>   KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks)
>
>   # disable stringop warnings in gcc 8+
> diff --git a/common/Kconfig b/common/Kconfig
> index 2bce8c9ba1..e30c3c4ab8 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -595,6 +595,21 @@ config TPL_HASH
>   	  and the algorithms it supports are defined in common/hash.c. See
>   	  also CMD_HASH for command-line access.
>
> +config STACKPROTECTOR
> +	bool "Stack Protector buffer overflow detection"
> +	default n
> +	help
> +	  Enable stack smash detection through gcc built-in stack-protector
> +	  canary logic
> +
> +config SPL_STACKPROTECTOR
> +	bool "Stack Protector buffer overflow detection for SPL"
> +	default n
> +
> +config TPL_STACKPROTECTOR
> +	bool "Stack Protector buffer overflow detection for SPL"

%s/SPL/TPL/

> +	default n
> +
>   endmenu
>
>   menu "Update support"
> diff --git a/common/Makefile b/common/Makefile
> index bcf352d016..fe71e18317 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -138,3 +138,5 @@ obj-$(CONFIG_CMD_LOADB) += xyzModem.o
>   obj-$(CONFIG_$(SPL_TPL_)YMODEM_SUPPORT) += xyzModem.o
>
>   obj-$(CONFIG_AVB_VERIFY) += avb_verify.o
> +obj-$(CONFIG_$(SPL_TPL_)STACKPROTECTOR) += stackprot.o
> +
> diff --git a/common/stackprot.c b/common/stackprot.c
> new file mode 100644
> index 0000000000..7c95b8544f
> --- /dev/null
> +++ b/common/stackprot.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Copyright 2021 Broadcom
> + */
> +
> +#include <common.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +unsigned long __stack_chk_guard = 0xfeedf00ddeadbeef;
> +
> +void __stack_chk_fail(void)

The standalone EFI binaries are compiled with -fstack-protector-strong
when selecting CONFIG_STACKPROTECTOR.

Do we need a function __stack_chk_fail) in
lib/efi_selftest/efi_freestanding.c and
lib/efi_loader/efi_freestanding.c too?

Could you, please, provide unit tests demonstrating that the stack
protection is actually working SPL, main U-Boot, and the EFI binaries.

Best regards

Heinrich

> +{
> +	panic("Stack smashing detected in function: %p relocated from %p",
> +	      __builtin_return_address(0),
> +	      __builtin_return_address(0) - gd->reloc_off);
> +}
> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
> index 9f1f7445d7..1505e4e851 100644
> --- a/scripts/Makefile.spl
> +++ b/scripts/Makefile.spl
> @@ -63,6 +63,12 @@ include $(srctree)/scripts/Makefile.lib
>   KBUILD_CFLAGS += -ffunction-sections -fdata-sections
>   LDFLAGS_FINAL += --gc-sections
>
> +ifeq ($(CONFIG_$(SPL_TPL_)STACKPROTECTOR),y)
> +KBUILD_CFLAGS += -fstack-protector-strong
> +else
> +KBUILD_CFLAGS += -fno-stack-protector
> +endif
> +
>   # FIX ME
>   cpp_flags := $(KBUILD_CPPFLAGS) $(PLATFORM_CPPFLAGS) $(UBOOTINCLUDE) \
>   							$(NOSTDINC_FLAGS)
>

Hi Heinrich,

  Thank you for your comments.  I have 2 questions about how to proceed....

1) Unit test
  I can add a function that can be used to trigger an overrun, but I am not
sure where to include it as the stack protector prints the error message
and then resets uboot so it wouldn't fir in a unit test suite.

  I could add a CONFIG_STACKPROTECTOR_TEST_FAIL to add a
"test_stackprotector fail" command to the CLI and you could call the
underlying stackprot_test_fail() from code hacked into SPL and TPL

2) Standalone/EFI
    What we did for our own standalone code was to add the KBUILD_CFLAGS +=
-fno-stack-protector   to the Makefile for our specific standalone.   The
problem is there is no generic place from which all standalone/EFI is
called, so I could just leave this for maintainers of specific
standalone/EPI programs to add IF they are enabling STACKPROTECTOR (If they
don't enable it, they don't need to do anything) or I could add
KBUILD_CFLAGS += -fno-stack-protector  to  both lib/efi_setlftest/Makefile
and lib/efi_loader/Makefile

What would you suggest?

Regards,

Joel

Am 10. Januar 2021 20:44:15 MEZ schrieb Joel Peshkin <joel.peshkin@broadcom.com>:
>Hi Heinrich,
>
>Thank you for your comments.  I have 2 questions about how to
>proceed....
>
>1) Unit test
>I can add a function that can be used to trigger an overrun, but I am
>not
>sure where to include it as the stack protector prints the error
>message
>and then resets uboot so it wouldn't fir in a unit test suite.
>
>  I could add a CONFIG_STACKPROTECTOR_TEST_FAIL to add a
>"test_stackprotector fail" command to the CLI and you could call the
>underlying stackprot_test_fail() from code hacked into SPL and TPL

Additonally to the test command you will nedd a Python test (in /test/py/tests/) to excercise it.


>
>2) Standalone/EFI
>What we did for our own standalone code was to add the KBUILD_CFLAGS +=
>-fno-stack-protector   to the Makefile for our specific standalone.  
>The
>problem is there is no generic place from which all standalone/EFI is
>called, so I could just leave this for maintainers of specific
>standalone/EPI programs to add IF they are enabling STACKPROTECTOR (If
>they
>don't enable it, they don't need to do anything) or I could add
>KBUILD_CFLAGS += -fno-stack-protector  to  both

This would lead to contradictory arguments on the GCC command line.

>lib/efi_setlftest/Makefile
>and lib/efi_loader/Makefile

Have a look at CFLAGS_REMOVE in aforementioned Makefiles. 

Best regards

Heinrich

>
>What would you suggest?
>
>Regards,
>
>Joel

Hi,
> +
> +unsigned long __stack_chk_guard = 0xfeedf00ddeadbeef;

sizeof(unsigned long) isn't always 8, even gcc issues a warning when it's invoked with proper options (e.g. 32-bit build):

> warning: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘18369602397475290863’ to ‘3735928559’ [-Woverflow]

Maybe there's some better way to initialize this variable. E.g. with #if … #else … #endif or using some initialization function that is invoked early.
I should also mention that a fixed canary value doesn't actually bring proper protection against exploits, thus run-time initialization with a random value is usually preferred.

I'm not sure whether it's important at all in bootloader code, I just wanted to be sure that it isn't unnoticed.

Cheers, Alex.

Hi Alex,

   Yeah, I think I'll wind up with some ifdef code for the static init.
 In the case of arm (32-bit), there is actually a GCC bug that causes it to
use the address of the canary value instead of the canary value itself.
GCC upstream just fixed that a few days ago, but it may be a year or so
before the appropriate GCC version is widely available.

    I may eventually add an optional mechanism to allow the value to be
changed (very carefully) at runtime.  This has to be done early enough that
we cannot wait for an RNG to be identified via DTB, but there are a few
ways to keep arm and aarch64 from being too predictable and some boards may
have peripherals that can provide a sufficiently variable value.

-Joel




On Sun, Jan 10, 2021 at 2:40 PM Alex Sadovsky <
nable.maininbox@googlemail.com> wrote:

> Hi,
> > +
> > +unsigned long __stack_chk_guard = 0xfeedf00ddeadbeef;
>
> sizeof(unsigned long) isn't always 8, even gcc issues a warning when it's
> invoked with proper options (e.g. 32-bit build):
>
> > warning: conversion from ‘long long unsigned int’ to ‘long unsigned int’
> changes value from ‘18369602397475290863’ to ‘3735928559’ [-Woverflow]
>
> Maybe there's some better way to initialize this variable. E.g. with #if …
> #else … #endif or using some initialization function that is invoked early.
> I should also mention that a fixed canary value doesn't actually bring
> proper protection against exploits, thus run-time initialization with a
> random value is usually preferred.
>
> I'm not sure whether it's important at all in bootloader code, I just
> wanted to be sure that it isn't unnoticed.
>
> Cheers, Alex.
>
>