[v2] 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>

Changes for v2:
   - Add test command and corresponding config
   - Fixed incorrect description in Kconfig
   - Add unit test
---
 Makefile                             |  4 +++
 common/Kconfig                       | 22 ++++++++++++++
 common/Makefile                      |  2 ++
 common/stackprot.c                   | 44 ++++++++++++++++++++++++++++
 configs/sandbox_defconfig            |  2 ++
 scripts/Makefile.spl                 |  6 ++++
 test/py/tests/test_stackprotector.py | 15 ++++++++++
 7 files changed, 95 insertions(+)
 create mode 100644 common/stackprot.c
 create mode 100644 test/py/tests/test_stackprotector.py

On 11.01.21 04:10, 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>
>
> Changes for v2:
>    - Add test command and corresponding config
>    - Fixed incorrect description in Kconfig
>    - Add unit test
> ---
>  Makefile                             |  4 +++
>  common/Kconfig                       | 22 ++++++++++++++
>  common/Makefile                      |  2 ++
>  common/stackprot.c                   | 44 ++++++++++++++++++++++++++++
>  configs/sandbox_defconfig            |  2 ++
>  scripts/Makefile.spl                 |  6 ++++
>  test/py/tests/test_stackprotector.py | 15 ++++++++++
>  7 files changed, 95 insertions(+)
>  create mode 100644 common/stackprot.c
>  create mode 100644 test/py/tests/test_stackprotector.py
>
> 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..f773babd3a 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -595,6 +595,28 @@ 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 STACKPROTECTOR_TEST_COMMAND

This should be called CMD_STACKPROTECTOR_TEST cmd/Kconfig.

> +	bool "Test command for stack protector"
> +	depends on STACKPROTECTOR
> +	default n
> +	help
> +	  Enable stackprot_test command
> +
> +config SPL_STACKPROTECTOR
> +	bool "Stack Protector buffer overflow detection for SPL"

depends on SPL && STACKPROTECTOR

> +	default n
> +
> +config TPL_STACKPROTECTOR
> +	bool "Stack Protector buffer overflow detection for TPL"

depends on TPL && STACKPROTECTOR

> +	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..d602dc7de1
> --- /dev/null
> +++ b/common/stackprot.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Copyright 2021 Broadcom
> + */
> +
> +#include <common.h>
> +#include <cli.h>
> +#include <command.h>
> +#include <console.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +unsigned long __stack_chk_guard =
> +    (unsigned long)((unsigned long long)0xfeedf00ddeadbeef &
> +		    ~((unsigned long)0));

Please, use scripts/checkpatch.pl and correct the reported problems, e.g.

WARNING: please, no spaces at the start of a line
#115: FILE: common/stackprot.c:14:
+    (unsigned long)((unsigned long long)0xfeedf00ddeadbeef &$

WARNING: Unnecessary typecast of c90 int constant
#115: FILE: common/stackprot.c:14:
+    (unsigned long)((unsigned long long)0xfeedf00ddeadbeef &

WARNING: Unnecessary typecast of c90 int constant
#116: FILE: common/stackprot.c:15:
+                   ~((unsigned long)0));

> +
> +void __stack_chk_fail(void)
> +{
> +	panic("Stack smashing detected in function: %p relocated from %p",
> +	      __builtin_return_address(0),
> +	      __builtin_return_address(0) - gd->reloc_off);
> +}
> +
> +#ifdef CONFIG_STACKPROTECTOR_TEST_COMMAND

The command should be in directory /cmd/. There you can use the Makefile
to decide if it is compiled.

Please, add a short man-page to /doc/usage/. See doc/usage/button.rst
for a template.

> +static int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int argc,
> +				  char *const argv[]);
> +
> +int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int argc,
> +				  char *const argv[])
> +{
> +	char a[128];
> +	char b[256];
> +	int i;

Missing empty line.

> +	for (i = 0; i < 256; i++) {
> +		b[i] = i;
> +	}
> +	memcpy(a, b, 512);

What is b[] good for? The following seems to be enough to do the test:

{
	char a[128];

	memset(a, 0xff, 512);
	return;
}

Best regards

Heinrich

> +	return (0);
> +}
> +
> +U_BOOT_CMD(stackprot_test, 1, 1, do_test_stackprot_fail,
> +	   "test stack protector fail", "");
> +
> +#endif
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index f1ec701a9f..da3aca2551 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -274,3 +274,5 @@ CONFIG_TEST_FDTDEC=y
>  CONFIG_UNIT_TEST=y
>  CONFIG_UT_TIME=y
>  CONFIG_UT_DM=y
> +CONFIG_STACKPROTECTOR=y
> +CONFIG_STACKPROTECTOR_TEST_COMMAND=y
> 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)
> diff --git a/test/py/tests/test_stackprotector.py b/test/py/tests/test_stackprotector.py
> new file mode 100644
> index 0000000000..49df8eff8c
> --- /dev/null
> +++ b/test/py/tests/test_stackprotector.py
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Broadcom
> +
> +import pytest
> +import signal
> +
> +@pytest.mark.buildconfigspec('stackprotector_test_command')
> +def test_stackprotector(u_boot_console):
> +    """Test that the stackprotector function works."""
> +
> +    u_boot_console.run_command('stackprot_test',wait_for_prompt=False)
> +    expected_response = 'Stack smashing detected'
> +    u_boot_console.wait_for(expected_response)
> +    assert(True)
> +
>