Message ID | 20210111162011.15115-1-joel.peshkin@broadcom.com |
---|---|
State | Changes Requested, archived |
Delegated to: | Tom Rini |
Headers | show |
Series | [v3] Add support for stack-protector | expand |
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) > + >
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> +#include <command.h> +#include <console.h> + +DECLARE_GLOBAL_DATA_PTR; + +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]; + + 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 + 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> + +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", + __builtin_return_address(0), + __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) +
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 ++++++ 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