[PATCH v9] Add support for stack-protector

Heinrich Schuchardt
2021-04-10T10:11:33Z

On 4/10/21 12:27 AM, Joel Peshkin wrote:
>
> Hi Heinrich,
>
> Has there been any progress in getting the EFI erors?fixed so that this
> can be committed?? There seems to be little point in my refreshing this
> patch until that is done.

I have fixed up your patch to work with EFI and will add it to my next
pull request.

I did not get qemu-x86_64_defconfig and qemu-x86_defconfig to build with
CONFIG_STACK_PROTECTOR=y, CONFIG_SPL_STACK_PROTECTOR = no.

arch/x86/cpu/irq.c and arch/x86/cpu/mtrr.c give me an undefined
reference to `__stack_chk_fail' when SPL is built. It seems that these
files are only built once instead of separately for SPL and main U-Boot.

Best regards

Heinrich

>
> Thanks,
>
> -Joel
>
>
> On Mon, Mar 22, 2021 at 10:37 AM Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
> 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
> <mailto:joel.peshkin@broadcom.com>>
> > ---
> > Cc: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto: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
> <mailto: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
> <mailto:bharat.gooty@broadcom.com>>
> >? M:? ?Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com
> <mailto: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
> > +
> > + at 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()
> > +
> >
>