[PATCH v10 0/3] Add support for stack-protector

[PATCH v10 0/3] Add support for stack-protector

[Heinrich Schuchardt]

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

Two preparatory patches are needed.
The first one avoids random failures of a unit test.
The second cleans up the usage of CFLAGS_NON_EFI.

The series was passes Gitlab CI:
https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/7133

v10:
	Preparatory patches.
	Fix build errors.

Heinrich Schuchardt (2):
  test: fix test/dm/regmap.c
  x86: correct usage of CFLAGS_NON_EFI

Joel Peshkin (1):
  Add support for stack-protector

 MAINTAINERS                          |  7 +++++++
 Makefile                             |  5 +++++
 arch/arm/config.mk                   |  3 ++-
 arch/riscv/lib/Makefile              |  1 +
 arch/x86/config.mk                   | 10 ++++++----
 cmd/Kconfig                          |  9 +++++++++
 cmd/Makefile                         |  1 +
 cmd/stackprot_test.c                 | 19 +++++++++++++++++++
 common/Kconfig                       | 17 +++++++++++++++++
 common/Makefile                      |  1 +
 common/stackprot.c                   | 20 ++++++++++++++++++++
 configs/sandbox_defconfig            |  2 ++
 scripts/Makefile.spl                 |  6 ++++++
 test/dm/regmap.c                     |  9 +++++----
 test/py/tests/test_stackprotector.py | 14 ++++++++++++++
 15 files changed, 115 insertions(+), 9 deletions(-)
 create mode 100644 cmd/stackprot_test.c
 create mode 100644 common/stackprot.c
 create mode 100644 test/py/tests/test_stackprotector.py

--
2.30.2

[PATCH v10 1/3] test: fix test/dm/regmap.c

[Heinrich Schuchardt]

regmap_read() only fills the first two bytes of val. The last two bytes are
random data from the stack. This means the test will fail randomly.

For low endian systems we could simply initialize val to 0 and get correct
results. But tests should not depend on endianness. So let's use a pointer
conversion instead.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v10:
	new patch
---
 test/dm/regmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/test/dm/regmap.c b/test/dm/regmap.c
index 22a293096c..372a73ca0c 100644
--- a/test/dm/regmap.c
+++ b/test/dm/regmap.c
@@ -286,7 +286,8 @@ U_BOOT_DRIVER(regmap_test) = {
 static int dm_test_devm_regmap(struct unit_test_state *uts)
 {
 	int i = 0;
-	u32 val;
+	u16 val;
+	void *valp = &val;
 	u16 pattern[REGMAP_TEST_BUF_SZ];
 	u16 *buffer;
 	struct udevice *dev;
@@ -311,7 +312,7 @@ static int dm_test_devm_regmap(struct unit_test_state *uts)
 		ut_assertok(regmap_write(priv->cfg_regmap, i, pattern[i]));
 	}
 	for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
-		ut_assertok(regmap_read(priv->cfg_regmap, i, &val));
+		ut_assertok(regmap_read(priv->cfg_regmap, i, valp));
 		ut_asserteq(val, buffer[i]);
 		ut_asserteq(val, pattern[i]);
 	}
@@ -319,9 +320,9 @@ static int dm_test_devm_regmap(struct unit_test_state *uts)
 	ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, REGMAP_TEST_BUF_SZ,
 					  val));
 	ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, REGMAP_TEST_BUF_SZ,
-					 &val));
+					 valp));
 	ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val));
-	ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val));
+	ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, valp));

 	return 0;
 }
--
2.30.2

[PATCH v10 2/3] x86: correct usage of CFLAGS_NON_EFI

[Heinrich Schuchardt]

The current usage of the variable CFLAGS_NON_EFI on the x86 architecture
deviates from other architectures.

Variable CFLAGS_NON_EFI is the list of compiler flags to be removed when
building UEFI applications. It is not a list of flags to be added anywhere.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v10:
	new patch
---
 arch/x86/config.mk | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 27d8412661..3067702858 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -39,10 +39,10 @@ LDFLAGS_EFI_PAYLOAD := -Bsymbolic -Bsymbolic-functions -shared --no-undefined -s
 OBJCOPYFLAGS_EFI := -j .text -j .sdata -j .data -j .dynamic -j .dynsym \
 	-j .rel -j .rela -j .reloc

-ifeq ($(IS_32BIT),y)
-CFLAGS_NON_EFI := -mregparm=3
-endif
+# Compiler flags to be added when building UEFI applications
 CFLAGS_EFI := -fpic -fshort-wchar
+# Compiler flags to be removed when building UEFI applications
+CFLAGS_NON_EFI := -mregparm=3

 ifeq ($(CONFIG_EFI_STUB_64BIT),)
 CFLAGS_EFI += $(call cc-option, -mno-red-zone)
@@ -70,7 +70,9 @@ LDSCRIPT := $(LDSCRIPT_EFI)

 else

-PLATFORM_CPPFLAGS += $(CFLAGS_NON_EFI)
+ifeq ($(IS_32BIT),y)
+PLATFORM_CPPFLAGS += -mregparm=3
+endif
 KBUILD_LDFLAGS += --emit-relocs
 LDFLAGS_FINAL += --gc-sections $(if $(CONFIG_SPL_BUILD),,-pie)

--
2.30.2

[PATCH v10 3/3] Add support for stack-protector

[Heinrich Schuchardt]

From: Joel Peshkin <joel.peshkin@broadcom.com>

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>

Adjust UEFI build flags.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v10:
	Adjust UEFI build flags.
	Remove unused DECLARE_GLOBAL_DATA_PTR in cmd/stackprot_test.c.
	Add missing include asm/global_data.h in common/stackprot.c.
---
 MAINTAINERS                          |  7 +++++++
 Makefile                             |  5 +++++
 arch/arm/config.mk                   |  3 ++-
 arch/riscv/lib/Makefile              |  1 +
 arch/x86/config.mk                   |  2 +-
 cmd/Kconfig                          |  9 +++++++++
 cmd/Makefile                         |  1 +
 cmd/stackprot_test.c                 | 19 +++++++++++++++++++
 common/Kconfig                       | 17 +++++++++++++++++
 common/Makefile                      |  1 +
 common/stackprot.c                   | 20 ++++++++++++++++++++
 configs/sandbox_defconfig            |  2 ++
 scripts/Makefile.spl                 |  6 ++++++
 test/py/tests/test_stackprotector.py | 14 ++++++++++++++
 14 files changed, 105 insertions(+), 2 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 c6dd9bf838..2d267aeff2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1062,6 +1062,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 193aa4d1c9..30565255b5 100644
--- a/Makefile
+++ b/Makefile
@@ -676,7 +676,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/arch/arm/config.mk b/arch/arm/config.mk
index 4153f7e371..e79f0104b9 100644
--- a/arch/arm/config.mk
+++ b/arch/arm/config.mk
@@ -11,7 +11,8 @@ CONFIG_STANDALONE_LOAD_ADDR = 0xc100000
 endif
 endif

-CFLAGS_NON_EFI := -fno-pic -ffixed-r9 -ffunction-sections -fdata-sections
+CFLAGS_NON_EFI := -fno-pic -ffixed-r9 -ffunction-sections -fdata-sections \
+		  -fstack-protector-strong
 CFLAGS_EFI := -fpic -fshort-wchar

 LDFLAGS_FINAL += --gc-sections
diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index ff0677aa96..d08cbe9b79 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_SPL_BUILD)	+= spl.o
 obj-y   += fdt_fixup.o

 # For building EFI apps
+CFLAGS_NON_EFI := -fstack-protector-strong
 CFLAGS_$(EFI_CRT0) := $(CFLAGS_EFI)
 CFLAGS_REMOVE_$(EFI_CRT0) := $(CFLAGS_NON_EFI)

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 3067702858..7a8242562d 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -42,7 +42,7 @@ OBJCOPYFLAGS_EFI := -j .text -j .sdata -j .data -j .dynamic -j .dynsym \
 # Compiler flags to be added when building UEFI applications
 CFLAGS_EFI := -fpic -fshort-wchar
 # Compiler flags to be removed when building UEFI applications
-CFLAGS_NON_EFI := -mregparm=3
+CFLAGS_NON_EFI := -mregparm=3 -fstack-protector-strong

 ifeq ($(CONFIG_EFI_STUB_64BIT),)
 CFLAGS_EFI += $(call cc-option, -mno-red-zone)
diff --git a/cmd/Kconfig b/cmd/Kconfig
index c735e81b37..420c1f0f94 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2324,6 +2324,15 @@ 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
+	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 e606ac4e8c..4977fa15f4 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -144,6 +144,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..36f5bac8d2
--- /dev/null
+++ b/cmd/stackprot_test.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Copyright 2021 Broadcom
+ */
+
+#include <common.h>
+#include <command.h>
+
+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 0e36dfd236..26496f9a2e 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -618,6 +618,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 215b8b26fd..bdad9655a9 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -137,4 +137,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
 obj-$(CONFIG_SCP03) += scp03.o
diff --git a/common/stackprot.c b/common/stackprot.c
new file mode 100644
index 0000000000..d5b7061665
--- /dev/null
+++ b/common/stackprot.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Copyright 2021 Broadcom
+ */
+
+#include <common.h>
+#include <asm/global_data.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 5da8d1679e..bc14da668d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -25,6 +25,7 @@ CONFIG_LOG_SYSLOG=y
 CONFIG_LOG_ERROR_RETURN=y
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_MISC_INIT_F=y
+CONFIG_STACKPROTECTOR=y
 CONFIG_ANDROID_AB=y
 CONFIG_CMD_CPU=y
 CONFIG_CMD_LICENSE=y
@@ -97,6 +98,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
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
index ca988224da..c69525fa74 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 0000000000..b009437e5e
--- /dev/null
+++ b/test/py/tests/test_stackprotector.py
@@ -0,0 +1,14 @@
+# 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()
--
2.30.2

[PATCH v10 1/3] test: fix test/dm/regmap.c

[Simon Glass]

On Sun, 11 Apr 2021 at 10:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> regmap_read() only fills the first two bytes of val. The last two bytes are
> random data from the stack. This means the test will fail randomly.
>
> For low endian systems we could simply initialize val to 0 and get correct
> results. But tests should not depend on endianness. So let's use a pointer
> conversion instead.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v10:
>         new patch
> ---
>  test/dm/regmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v10 2/3] x86: correct usage of CFLAGS_NON_EFI

[Simon Glass]

Hi Heinrich,

On Sun, 11 Apr 2021 at 10:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The current usage of the variable CFLAGS_NON_EFI on the x86 architecture
> deviates from other architectures.
>
> Variable CFLAGS_NON_EFI is the list of compiler flags to be removed when
> building UEFI applications. It is not a list of flags to be added anywhere.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v10:
>         new patch
> ---
>  arch/x86/config.mk | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Well it used to be the flags to add when not building EFI apps... I
suppose it doesn't matter so long as it works.

Regards,
Simon

[PATCH v10 1/3] test: fix test/dm/regmap.c

[Tom Rini]

On Sun, Apr 11, 2021 at 11:21:56AM +0200, Heinrich Schuchardt wrote:

> regmap_read() only fills the first two bytes of val. The last two bytes are
> random data from the stack. This means the test will fail randomly.
> 
> For low endian systems we could simply initialize val to 0 and get correct
> results. But tests should not depend on endianness. So let's use a pointer
> conversion instead.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210420/e5131f9d/attachment.sig>

[PATCH v10 2/3] x86: correct usage of CFLAGS_NON_EFI

[Tom Rini]

On Sun, Apr 11, 2021 at 11:21:57AM +0200, Heinrich Schuchardt wrote:

> The current usage of the variable CFLAGS_NON_EFI on the x86 architecture
> deviates from other architectures.
> 
> Variable CFLAGS_NON_EFI is the list of compiler flags to be removed when
> building UEFI applications. It is not a list of flags to be added anywhere.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210420/6a7f5cc0/attachment.sig>

[PATCH v10 3/3] Add support for stack-protector

[Tom Rini]

On Sun, Apr 11, 2021 at 11:21:58AM +0200, Heinrich Schuchardt wrote:

> From: Joel Peshkin <joel.peshkin@broadcom.com>
> 
> 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>
> 
> Adjust UEFI build flags.
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210420/b7bb4043/attachment.sig>