[PATCH v9] Add support for stack-protector

Heinrich Schuchardt xypron.glpk at gmx.de
Sat Apr 10 12:11:33 CEST 2021


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 at gmx.de
> <mailto:xypron.glpk at 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 at broadcom.com
>     <mailto:joel.peshkin at broadcom.com>>
>      > ---
>      > Cc: Simon Glass <sjg at chromium.org <mailto:sjg at chromium.org>>
>      > Cc: Heinrich Schuchardt <xypron.glpk at gmx.de
>     <mailto:xypron.glpk at 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 at broadcom.com
>     <mailto:joel.peshkin at 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 at broadcom.com
>     <mailto:bharat.gooty at broadcom.com>>
>      >  M:   Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com
>     <mailto:rayagonda.kokatanur at 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()
>      > +
>      >
>



More information about the U-Boot mailing list