| Message ID | 20210114203535.134138-1-joel.peshkin@broadcom.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v8] Add support for stack-protector | expand |
On 14.01.21 21:35, 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> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > 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
On Thu, Jan 14, 2021 at 12:35:35PM -0800, 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> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Oddly enough this cases a number of the TPM2 tests to fail on sandbox: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=1706&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=755
On 1/28/21 1:57 AM, Tom Rini wrote: > On Thu, Jan 14, 2021 at 12:35:35PM -0800, 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> >> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Oddly enough this cases a number of the TPM2 tests to fail on sandbox: > https://dev.azure.com/u-boot/u-boot/_build/results?buildId=1706&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=755 > Thanks Tom for the head-up. The problem is reproducible locally using 'make tests' with the patch applied on top of origin/master. Best regards Heinrich
On 28.01.21 09:20, Heinrich Schuchardt wrote: > On 1/28/21 1:57 AM, Tom Rini wrote: >> On Thu, Jan 14, 2021 at 12:35:35PM -0800, 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> >>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> >> Oddly enough this cases a number of the TPM2 tests to fail on sandbox: >> https://dev.azure.com/u-boot/u-boot/_build/results?buildId=1706&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=755 >> >> > > Thanks Tom for the head-up. > > The problem is reproducible locally using 'make tests' with the patch > applied on top of origin/master. > > Best regards > > Heinrich The stack protector has successfully detected stack smashes. See below. A secondary finding is: panic("foo") without \n is not correctly flushed to the console on the sandbox. test_tpm2_init: Stack smashing detected in function: 00005563faa7c521 relocated from 000000000004d521 u-boot.map 0x000000000004cf44 do_spi 0x000000000004d9e9 print_byte_string It seems that the output written in the stack-protector test ends up in the following test which happens to be test_tpm2_init: static int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { 4d4db: 48 81 ec 98 00 00 00 sub $0x98,%rsp char a[128]; memset(a, 0xa5, 512); 4d4e2: ba 00 02 00 00 mov $0x200,%edx 4d4e7: be a5 00 00 00 mov $0xa5,%esi { 4d4ec: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax 4d4f3: 00 00· 4d4f5: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp) 4d4fc: 00· 4d4fd: 31 c0 xor %eax,%eax memset(a, 0xa5, 512); 4d4ff: 48 8d 7c 24 08 lea 0x8(%rsp),%rdi 4d504: e8 1f 34 0d 00 callq 120928 <memset> return 0; } 4d509: 48 8b 84 24 88 00 00 mov 0x88(%rsp),%rax 4d510: 00· 4d511: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax 4d518: 00 00· 4d51a: 74 05 je 4d521 <do_test_stackprot_fail+0x46> 4d51c: e8 68 48 02 00 callq 71d89 <__stack_chk_fail> >>>4d521: 31 c0 xor %eax,%eax 4d523: 48 81 c4 98 00 00 00 add $0x98,%rsp 4d52a: c3 retq··· TestEfiCapsuleFirmwareFit.test_efi_capsule_fw3 #Stack smashing detected in function: 000055827c21cb58 relocated from 00000000000d8b58 Here we seem to have a real finding. u-boot.map 0x00000000000d7fae efi_launch_capsules 0x00000000000da1ec set_shift_mask efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report, d8b10: 4c 8b 04 24 mov (%rsp),%r8 d8b14: 45 31 c9 xor %r9d,%r9d d8b17: 48 8d 35 42 1c 11 00 lea 0x111c42(%rip),%rsi # 1ea760 <efi_guid_capsule_report> d8b1e: b9 16 00 00 00 mov $0x16,%ecx d8b23: ba 07 00 00 80 mov $0x80000007,%edx d8b28: 48 8d 3d e7 61 0c 00 lea 0xc61e7(%rip),%rdi # 19ed16 <__func__.0+0x132e6> d8b2f: e8 34 ab 00 00 callq e3668 <efi_set_variable_int> return ret; d8b34: eb 0a jmp d8b40 <efi_launch_capsules+0xb92> return EFI_DEVICE_ERROR; d8b36: 49 bc 07 00 00 00 00 movabs $0x8000000000000007,%r12 d8b3d: 00 00 80· } d8b40: 48 8b 84 24 c8 00 00 mov 0xc8(%rsp),%rax d8b47: 00· d8b48: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax d8b4f: 00 00· d8b51: 74 05 je d8b58 <efi_launch_capsules+0xbaa> d8b53: e8 31 92 f9 ff callq 71d89 <__stack_chk_fail> >>>d8b58: 48 81 c4 d8 00 00 00 add $0xd8,%rsp d8b5f: 4c 89 e0 mov %r12,%rax d8b62: 5b pop %rbx d8b63: 5d pop %rbp d8b64: 41 5c pop %r12 d8b66: 41 5d pop %r13 d8b68: 41 5e pop %r14 d8b6a: 41 5f pop %r15 d8b6c: c3 retq··· Best regards Heinrich
On 28.01.21 12:00, Heinrich Schuchardt wrote: > On 28.01.21 09:20, Heinrich Schuchardt wrote: >> On 1/28/21 1:57 AM, Tom Rini wrote: >>> On Thu, Jan 14, 2021 at 12:35:35PM -0800, 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> >>>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> >>> Oddly enough this cases a number of the TPM2 tests to fail on sandbox: >>> https://dev.azure.com/u-boot/u-boot/_build/results?buildId=1706&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=755 >>> >>> >> >> Thanks Tom for the head-up. >> >> The problem is reproducible locally using 'make tests' with the patch >> applied on top of origin/master. >> >> Best regards >> >> Heinrich > > The stack protector has successfully detected stack smashes. See below. > > A secondary finding is: panic("foo") without \n is not correctly flushed > to the console on the sandbox. ./test/py/test.py --bd=sandbox -ra --build-dir=build-sandbox -k='not test_stackprotector' shows no error for tpm2. ./test/py/test.py --bd=sandbox -ra --build-dir=build-sandbox -k='not test_stackprotector' shows the error for tpm2. I think the test test_stackprotector() is broken. I causes the sandbox to reset but does not wait until reaches the prompt again, e.g add u_boot_console.restart_uboot() instead of assert(true). @Joel Could you, please, submit a corrected patch. @Takahiro For test/py/tests/test_efi_capsule/test_capsule_firmware.py I always see a smash. Could you, please, have a look at efi_launch_capsules(). Best regards Heinrich > > test_tpm2_init: > > Stack smashing detected in function: > 00005563faa7c521 relocated from 000000000004d521 > > u-boot.map > 0x000000000004cf44 do_spi > 0x000000000004d9e9 print_byte_string > > It seems that the output written in the stack-protector test ends up in > the following test which happens to be test_tpm2_init: > > static int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > { > 4d4db: 48 81 ec 98 00 00 00 sub $0x98,%rsp > char a[128]; > > memset(a, 0xa5, 512); > 4d4e2: ba 00 02 00 00 mov $0x200,%edx > 4d4e7: be a5 00 00 00 mov $0xa5,%esi > { > 4d4ec: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax > 4d4f3: 00 00· > 4d4f5: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp) > 4d4fc: 00· > 4d4fd: 31 c0 xor %eax,%eax > memset(a, 0xa5, 512); > 4d4ff: 48 8d 7c 24 08 lea 0x8(%rsp),%rdi > 4d504: e8 1f 34 0d 00 callq 120928 <memset> > return 0; > } > 4d509: 48 8b 84 24 88 00 00 mov 0x88(%rsp),%rax > 4d510: 00· > 4d511: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax > 4d518: 00 00· > 4d51a: 74 05 je 4d521 > <do_test_stackprot_fail+0x46> > 4d51c: e8 68 48 02 00 callq 71d89 <__stack_chk_fail> >>>> 4d521: 31 c0 xor %eax,%eax > 4d523: 48 81 c4 98 00 00 00 add $0x98,%rsp > 4d52a: c3 retq··· > > > > TestEfiCapsuleFirmwareFit.test_efi_capsule_fw3 > #Stack smashing detected in function: > 000055827c21cb58 relocated from 00000000000d8b58 > > Here we seem to have a real finding. > > u-boot.map > 0x00000000000d7fae efi_launch_capsules > 0x00000000000da1ec set_shift_mask > > efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report, > d8b10: 4c 8b 04 24 mov (%rsp),%r8 > d8b14: 45 31 c9 xor %r9d,%r9d > d8b17: 48 8d 35 42 1c 11 00 lea 0x111c42(%rip),%rsi > # 1ea760 <efi_guid_capsule_report> > d8b1e: b9 16 00 00 00 mov $0x16,%ecx > d8b23: ba 07 00 00 80 mov $0x80000007,%edx > d8b28: 48 8d 3d e7 61 0c 00 lea 0xc61e7(%rip),%rdi > # 19ed16 <__func__.0+0x132e6> > d8b2f: e8 34 ab 00 00 callq e3668 <efi_set_variable_int> > return ret; > d8b34: eb 0a jmp d8b40 > <efi_launch_capsules+0xb92> > return EFI_DEVICE_ERROR; > d8b36: 49 bc 07 00 00 00 00 movabs $0x8000000000000007,%r12 > d8b3d: 00 00 80· > } > d8b40: 48 8b 84 24 c8 00 00 mov 0xc8(%rsp),%rax > d8b47: 00· > d8b48: 64 48 2b 04 25 28 00 sub %fs:0x28,%rax > d8b4f: 00 00· > d8b51: 74 05 je d8b58 > <efi_launch_capsules+0xbaa> > d8b53: e8 31 92 f9 ff callq 71d89 <__stack_chk_fail> >>>> d8b58: 48 81 c4 d8 00 00 00 add $0xd8,%rsp > d8b5f: 4c 89 e0 mov %r12,%rax > d8b62: 5b pop %rbx > d8b63: 5d pop %rbp > d8b64: 41 5c pop %r12 > d8b66: 41 5d pop %r13 > d8b68: 41 5e pop %r14 > d8b6a: 41 5f pop %r15 > d8b6c: c3 retq··· > > > Best regards > > Heinrich >
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; + +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> + +DECLARE_GLOBAL_DATA_PTR; + +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..957a2a6 --- /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) +
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 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