[PATCH v6] Add support for stack-protector

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 12.01.21 17:51, Joel Peshkin wrote:> Cc: Simon Glass
<sjg@chromium.org>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>>
Signed-off-by: Joel Peshkin <joel.peshkin@broadcom.com>
These lines should be below the commit message body.

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

The following lines should be below the first ---.

> 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                   | 17 +++++++++++++++++
>  configs/sandbox_defconfig            | 14 +++++++-------
>  scripts/Makefile.spl                 |  6 ++++++
>  test/py/tests/test_stackprotector.py | 15 +++++++++++++++
>  11 files changed, 108 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;
> +
> +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..1f1c3b4
> --- /dev/null
> +++ b/common/stackprot.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Copyright 2021 Broadcom
> + */
> +
> +#include <common.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +unsigned long __stack_chk_guard = (long)(0xfeedf00ddeadbeef & ~0L);
> +
> +void __stack_chk_fail(void)
> +{
> +	panic("Stack smashing detected in function:\n%p relocated from %p",
> +	      __builtin_extract_return_addr(__builtin_return_address(0)),
> +	      __builtin_return_address(0) - gd->reloc_off);

A __builtin_extract_return_addr() is missing here.

I suggest that you first write
__builtin_extract_return_addr(__builtin_return_address(0))
into a variable and than use it in panic().

Best regards

Heinrich

> +}
> 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
> +
> + 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)
> +    assert(True)
> +
>