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

On 09.02.21 04:36, Joel Peshkin wrote:

Add support for stack protector for UBOOT, SPL, and TPL as well as new pytest for stackprotector

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

Cc: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de

Changes for v9:

• Fix pytest script post-test reboot

Changes for v8:

• Fix commit message
• Force canary to UL type

Changes for v7:

• Fix commit message
• add __builtin_extract_return_addr() calls

Changes for v6:

• Fix commit message

Changes for v5:

• Rebase

Changes for v4:

• Exclude EFI from stackprotector
• Cleanups of extra includes and declaration

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 | 5 +++++ cmd/Kconfig | 10 ++++++++++ cmd/Makefile | 1 + cmd/stackprot_test.c | 21 +++++++++++++++++++++ common/Kconfig | 17 +++++++++++++++++ common/Makefile | 2 ++ common/stackprot.c | 19 +++++++++++++++++++ configs/sandbox_defconfig | 14 +++++++------- scripts/Makefile.spl | 6 ++++++ test/py/tests/test_stackprotector.py | 15 +++++++++++++++ 11 files changed, 110 insertions(+), 7 deletions(-) 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 26dd254..d3971e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1024,6 +1024,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 902a976..65c5cb8 100644 --- a/Makefile +++ b/Makefile @@ -677,7 +677,12 @@ else KBUILD_CFLAGS += -O2 endif

+ifeq ($(CONFIG_STACKPROTECTOR),y) +KBUILD_CFLAGS += $(call cc-option,-fstack-protector-strong) +CFLAGS_EFI += $(call cc-option,-fno-stack-protector) +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 da86a94..054b2f3 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -2280,6 +2280,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 5b3400a..1d7afea 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 0000000..6ad287e --- /dev/null +++ b/cmd/stackprot_test.c @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2021 Broadcom
• */

+#include <common.h> +#include <command.h>

+DECLARE_GLOBAL_DATA_PTR;

Hello Joël,

This line is not needed.

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

• 		  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 2bce8c9..6a94045 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 compiler's stack-protector
  

•  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 bcf352d..fe71e18 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 0000000..282c564 --- /dev/null +++ b/common/stackprot.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2021 Broadcom
• */

+#include <common.h>

You need

#include <asm/global_data.h>

here due to recently merged restructuring of includes by Simon.

+DECLARE_GLOBAL_DATA_PTR;

common/stackprot.c:8:1: warning: data definition has no type or storage class 8 | DECLARE_GLOBAL_DATA_PTR; | ^~~~~~~~~~~~~~~~~~~~~~~ common/stackprot.c:8:1: warning: type defaults to ‘int’ in declaration of ‘DECLARE_GLOBAL_DATA_PTR’ [-Wimplicit-int] common/stackprot.c: In function ‘__stack_chk_fail’: common/stackprot.c:18:17: error: ‘gd’ undeclared (first use in this function) 18 | ra, ra - gd->reloc_off);

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

+void __stack_chk_fail(void) +{

• void *ra;
• ra = __builtin_extract_return_addr(__builtin_return_address(0));
• panic("Stack smashing detected in function:\n%p relocated from %p",

•      ra, ra - gd->reloc_off);
  

+} diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 58d4ef1..0c82ef2 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -20,11 +20,11 @@ CONFIG_BOOTSTAGE_STASH=y CONFIG_BOOTSTAGE_STASH_SIZE=0x4096 CONFIG_CONSOLE_RECORD=y CONFIG_CONSOLE_RECORD_OUT_SIZE=0x1000 -CONFIG_SILENT_CONSOLE=y CONFIG_PRE_CONSOLE_BUFFER=y CONFIG_LOG_SYSLOG=y CONFIG_LOG_ERROR_RETURN=y CONFIG_DISPLAY_BOARDINFO_LATE=y +CONFIG_STACKPROTECTOR=y CONFIG_ANDROID_AB=y CONFIG_CMD_CPU=y CONFIG_CMD_LICENSE=y @@ -96,6 +96,7 @@ CONFIG_CMD_CRAMFS=y CONFIG_CMD_EXT4_WRITE=y CONFIG_CMD_SQUASHFS=y CONFIG_CMD_MTDPARTS=y +CONFIG_CMD_STACKPROTECTOR_TEST=y CONFIG_MAC_PARTITION=y CONFIG_AMIGA_PARTITION=y CONFIG_OF_CONTROL=y @@ -131,6 +132,7 @@ CONFIG_CPU=y CONFIG_DM_DEMO=y CONFIG_DM_DEMO_SIMPLE=y CONFIG_DM_DEMO_SHAPE=y +CONFIG_DFU_SF=y CONFIG_DMA=y CONFIG_DMA_CHANNELS=y CONFIG_SANDBOX_DMA=y @@ -269,14 +271,12 @@ CONFIG_CMD_DHRYSTONE=y CONFIG_TPM=y CONFIG_LZ4=y CONFIG_ERRNO_STR=y +CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y +CONFIG_EFI_CAPSULE_ON_DISK=y +CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y CONFIG_EFI_SECURE_BOOT=y CONFIG_TEST_FDTDEC=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y -# -CONFIG_DFU_SF=y -CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y -CONFIG_EFI_CAPSULE_ON_DISK=y -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y -CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 87021e2..6725201 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -67,6 +67,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 0000000..7aeec5e --- /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)
• u_boot_console.restart_uboot()