Re: [PATCH v3] Add support for stack-protector

11 Jan 2021

On 11.01.21 17:20, 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 v3:

Move test command to cmd/
Update Kconfig names and depends
clean up default canary initialization

Changes for v2:

Add test command and corresponding config
Fixed incorrect description in Kconfig
Add unit test


MAINTAINERS                          |  7 +++++++
 Makefile                             |  4 ++++
 cmd/Kconfig                          | 10 ++++++++++
 cmd/Makefile                         |  1 +
 cmd/stackprot_test.c                 | 26 ++++++++++++++++++++++++++
 common/Kconfig                       | 17 +++++++++++++++++
 common/Makefile                      |  2 ++
 common/stackprot.c                   | 18 ++++++++++++++++++
 configs/sandbox_defconfig            |  2 ++
 scripts/Makefile.spl                 |  6 ++++++
Thanks for resubmitting.
I am missing the changes for lib/efi_loader/Makefile and
lib/efi_selftest/Makefile to keep the stack protector out of the EFI
binaries.
...
test/py/tests/test_stackprotector.py | 15 +++++++++++++++
 11 files changed, 108 insertions(+)
 create mode 100644 cmd/stackprot_test.c
 create mode 100644 common/stackprot.c
 create mode 100644 test/py/tests/test_stackprotector.py

diff --git a/MAINTAINERS b/MAINTAINERS
index 52d7307525..2982ffcc07 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1022,6 +1022,13 @@ F:	include/sqfs.h
 F:	cmd/sqfs.c
 F:	test/py/tests/test_fs/test_squashfs/
+STACKPROTECTOR
+M:	Joel Peshkin joel.peshkin@broadcom.com
+S:	Maintained
+F:	common/stackprot.c
+F:	cmd/stackprot_test.c
+F:	test/py/tests/test_stackprotector.py



TARGET_BCMNS3
 M:	Bharat Gooty bharat.gooty@broadcom.com
 M:	Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com
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/cmd/Kconfig b/cmd/Kconfig
index 1595de999b..04700e4bb4 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2271,6 +2271,16 @@ config CMD_AVB
       avb read_part_hex - read data from partition and output to stdout
       avb write_part - write data to partition
       avb verify - run full verification chain



+config CMD_STACKPROTECTOR_TEST

bool "Test command for stack protector"
depends on STACKPROTECTOR
default n
help
 Enable stackprot_test command


 The stackprot_test command will force a stack overrun to test


 the stack smashing detection mechanisms.




endmenu
config CMD_UBI
diff --git a/cmd/Makefile b/cmd/Makefile
index dd86675bf2..8e8fcea19e 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -142,6 +142,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o
 obj-$(CONFIG_CMD_STRINGS) += strings.o
 obj-$(CONFIG_CMD_SMC) += smccc.o
 obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o pxe_utils.o
+obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
 obj-$(CONFIG_CMD_TERMINAL) += terminal.o
 obj-$(CONFIG_CMD_TIME) += time.o
 obj-$(CONFIG_CMD_TIMER) += timer.o
diff --git a/cmd/stackprot_test.c b/cmd/stackprot_test.c
new file mode 100644
index 0000000000..ea952c075e
--- /dev/null
+++ b/cmd/stackprot_test.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*


Copyright 2021 Broadcom


*/


+#include <common.h>
+#include <cli.h>
The include is not needed and should be removed.
...
+#include <command.h>
+#include <console.h>
superfluous include
...



+DECLARE_GLOBAL_DATA_PTR;



+static int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int argc,

		  char *const argv[]);



superfluous forward declaration
...



+int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int argc,
The static keyword can be added here.
Static is preferable. The only good thing about making it non-static
would be easy checking if the address output works. Looks good at first
sight:
Stack smashing detected for function:
00005591a897ea34 relocated from 000000000004ba34
u-boot.map:
0x000000000004b9ee do_test_stackprot_fail
0x000000000004befc print_byte_string
...

	   char *const argv[])



+{

char a[128];

memset(a, 0xa5, 512);
return 0;

+}



+U_BOOT_CMD(stackprot_test, 1, 1, do_test_stackprot_fail,

  "test stack protector fail", "");



diff --git a/common/Kconfig b/common/Kconfig
index 2bce8c9ba1..5a053c7336 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -595,6 +595,23 @@ 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



We build U-Boot with Clang too. So I would prefer not to reference any
individual compiler here.
GCC would have to be capitalized.
...

 canary logic




+config SPL_STACKPROTECTOR

bool "Stack Protector buffer overflow detection for SPL"
depends on STACKPROTECTOR && SPL
default n


+config TPL_STACKPROTECTOR

bool "Stack Protector buffer overflow detection for TPL"
depends on STACKPROTECTOR && 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..014257b3e7
--- /dev/null
+++ b/common/stackprot.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*


Copyright 2021 Broadcom


*/


+#include <common.h>
+#include <console.h>
superfluous include
...



+DECLARE_GLOBAL_DATA_PTR;



+unsigned long __stack_chk_guard = (long)(0xfeedf00ddeadbeef & ~0L);



+void __stack_chk_fail(void)
+{

panic("Stack smashing detected in function: %p relocated from %p",

On a 64-bit system the current output does not fit into 80 characters.
Please add a newline after the colon.
...

     __builtin_return_address(0),



Shouldn't you use
__builtin_extract_return_addr(__builtin_return_address(0));
For instance on ARM6 condition codes may have to be masked out of the
return address.
Cf.
https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
https://github.com/gcc-mirror/gcc/blob/master/gcc/config/arm/arm.h#L2316
Best regards
Heinrich
...

     __builtin_return_address(0) - gd->reloc_off);



+}
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index f1ec701a9f..3e92a58bdb 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_CMD_STACKPROTECTOR_TEST=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..957a2a6cf7
--- /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('cmd_stackprotector_test')
+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)



    