[PATCH v2 0/8] tools/nolibc: add support for stack protector

[PATCH v2 0/8] tools/nolibc: add support for stack protector

[Thomas Weißschuh]

This is useful when using nolibc for security-critical tools.
Using nolibc has the advantage that the code is easily auditable and
sandboxable with seccomp as no unexpected syscalls are used.
Using compiler-assistent stack protection provides another security
mechanism.

For this to work the compiler and libc have to collaborate.

This patch adds the following parts to nolibc that are required by the
compiler:

* __stack_chk_guard: random sentinel value
* __stack_chk_fail: handler for detected stack smashes

In addition an initialization function is added that randomizes the
sentinel value.

Only support for global guards is implemented.
Register guards are useful in multi-threaded context which nolibc does
not provide support for.

Link: https://lwn.net/Articles/584225/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- Code and comments style fixes
- Only use raw syscalls in stackprotector functions
- Remove need for dedicated entrypoint and exec() during tests
- Add more rationale
- Shuffle some code around between commits
- Provide compatibility with the -fno-stack-protector patch
- Remove RFC status
- Link to v1: https://lore.kernel.org/r/20230223-nolibc-stackprotector-v1-0-3e74d81b3f21@weissschuh.net

This series is based on the current rcu/dev branch of Pauls rcu tree.

---
Thomas Weißschuh (8):
      tools/nolibc: add definitions for standard fds
      tools/nolibc: add helpers for wait() signal exits
      tools/nolibc: tests: constify test_names
      tools/nolibc: add support for stack protector
      tools/nolibc: tests: fold in no-stack-protector cflags
      tools/nolibc: tests: add test for -fstack-protector
      tools/nolibc: i386: add stackprotector support
      tools/nolibc: x86_64: add stackprotector support

 tools/include/nolibc/Makefile                |  4 +-
 tools/include/nolibc/arch-i386.h             |  7 ++-
 tools/include/nolibc/arch-x86_64.h           |  5 +++
 tools/include/nolibc/nolibc.h                |  1 +
 tools/include/nolibc/stackprotector.h        | 53 +++++++++++++++++++++++
 tools/include/nolibc/types.h                 |  2 +
 tools/include/nolibc/unistd.h                |  5 +++
 tools/testing/selftests/nolibc/Makefile      | 11 ++++-
 tools/testing/selftests/nolibc/nolibc-test.c | 64 ++++++++++++++++++++++++++--
 9 files changed, 144 insertions(+), 8 deletions(-)
---
base-commit: a9b8406e51603238941dbc6fa1437f8915254ebb
change-id: 20230223-nolibc-stackprotector-d4d5f48ff771

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 1/8] tools/nolibc: add definitions for standard fds

[Thomas Weißschuh]

These are useful for users and will also be used in an upcoming
testcase.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/unistd.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/include/nolibc/unistd.h b/tools/include/nolibc/unistd.h
index 1cfcd52106a4..ac7d53d986cd 100644
--- a/tools/include/nolibc/unistd.h
+++ b/tools/include/nolibc/unistd.h
@@ -13,6 +13,11 @@
 #include "sys.h"
 
 
+#define STDIN_FILENO  0
+#define STDOUT_FILENO 1
+#define STDERR_FILENO 2
+
+
 static __attribute__((unused))
 int msleep(unsigned int msecs)
 {

-- 
2.40.0

[PATCH v2 2/8] tools/nolibc: add helpers for wait() signal exits

[Thomas Weißschuh]

These are useful for users and will also be used in an upcoming
testcase.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/types.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/include/nolibc/types.h b/tools/include/nolibc/types.h
index 10823e5ac44b..aedd7d9e3f64 100644
--- a/tools/include/nolibc/types.h
+++ b/tools/include/nolibc/types.h
@@ -97,6 +97,8 @@
 /* Macros used on waitpid()'s return status */
 #define WEXITSTATUS(status) (((status) & 0xff00) >> 8)
 #define WIFEXITED(status)   (((status) & 0x7f) == 0)
+#define WTERMSIG(status)    ((status) & 0x7f)
+#define WIFSIGNALED(status) ((status) - 1 < 0xff)
 
 /* waitpid() flags */
 #define WNOHANG      1

-- 
2.40.0

[PATCH v2 3/8] tools/nolibc: tests: constify test_names

[Thomas Weißschuh]

Nothing ever modifies this structure.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 6a7c13f0cd61..fb2d4872fac9 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -717,7 +717,7 @@ int prepare(void)
 }
 
 /* This is the definition of known test names, with their functions */
-static struct test test_names[] = {
+static const struct test test_names[] = {
 	/* add new tests here */
 	{ .name = "syscall",   .func = run_syscall  },
 	{ .name = "stdlib",    .func = run_stdlib   },

-- 
2.40.0

[PATCH v2 4/8] tools/nolibc: add support for stack protector

[Thomas Weißschuh]

This is useful when using nolibc for security-critical tools.
Using nolibc has the advantage that the code is easily auditable and
sandboxable with seccomp as no unexpected syscalls are used.
Using compiler-assistent stack protection provides another security
mechanism.

For this to work the compiler and libc have to collaborate.

This patch adds the following parts to nolibc that are required by the
compiler:

* __stack_chk_guard: random sentinel value
* __stack_chk_fail: handler for detected stack smashes

In addition an initialization function is added that randomizes the
sentinel value.

Only support for global guards is implemented.
Register guards are useful in multi-threaded context which nolibc does
not provide support for.

Link: https://lwn.net/Articles/584225/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/Makefile         |  4 +--
 tools/include/nolibc/nolibc.h         |  1 +
 tools/include/nolibc/stackprotector.h | 53 +++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
index ec57d3932506..9839feafd38a 100644
--- a/tools/include/nolibc/Makefile
+++ b/tools/include/nolibc/Makefile
@@ -25,8 +25,8 @@ endif
 
 nolibc_arch := $(patsubst arm64,aarch64,$(ARCH))
 arch_file := arch-$(nolibc_arch).h
-all_files := ctype.h errno.h nolibc.h signal.h std.h stdint.h stdio.h stdlib.h \
-             string.h sys.h time.h types.h unistd.h
+all_files := ctype.h errno.h nolibc.h signal.h stackprotector.h std.h stdint.h \
+             stdio.h stdlib.h string.h sys.h time.h types.h unistd.h
 
 # install all headers needed to support a bare-metal compiler
 all: headers
diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index b2bc48d3cfe4..04739a6293c4 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -104,6 +104,7 @@
 #include "string.h"
 #include "time.h"
 #include "unistd.h"
+#include "stackprotector.h"
 
 /* Used by programs to avoid std includes */
 #define NOLIBC
diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h
new file mode 100644
index 000000000000..d119cbbbc256
--- /dev/null
+++ b/tools/include/nolibc/stackprotector.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
+/*
+ * Stack protector support for NOLIBC
+ * Copyright (C) 2023 Thomas Weißschuh <linux@weissschuh.net>
+ */
+
+#ifndef _NOLIBC_STACKPROTECTOR_H
+#define _NOLIBC_STACKPROTECTOR_H
+
+#include "arch.h"
+
+#if defined(NOLIBC_STACKPROTECTOR)
+
+#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR)
+#error "nolibc does not support stack protectors on this arch"
+#endif
+
+#include "sys.h"
+#include "stdlib.h"
+
+/* The functions in this header are using raw syscall macros to avoid
+ * triggering stack protector errors themselves
+ */
+
+__attribute__((weak,noreturn,section(".text.nolibc_stack_chk")))
+void __stack_chk_fail(void)
+{
+	pid_t pid;
+	my_syscall3(__NR_write, STDERR_FILENO, "!!Stack smashing detected!!\n", 28);
+	pid = my_syscall0(__NR_getpid);
+	my_syscall2(__NR_kill, pid, SIGABRT);
+	for (;;);
+}
+
+__attribute__((weak,noreturn,section(".text.nolibc_stack_chk")))
+void __stack_chk_fail_local(void)
+{
+	__stack_chk_fail();
+}
+
+__attribute__((weak,section(".data.nolibc_stack_chk")))
+uintptr_t __stack_chk_guard;
+
+__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk")))
+void __stack_chk_init(void)
+{
+	my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0);
+	/* a bit more randomness in case getrandom() fails */
+	__stack_chk_guard ^= (uintptr_t) &__stack_chk_guard;
+}
+#endif // defined(NOLIBC_STACKPROTECTOR)
+
+#endif // _NOLIBC_STACKPROTECTOR_H

-- 
2.40.0

[PATCH v2 5/8] tools/nolibc: tests: fold in no-stack-protector cflags

[Thomas Weißschuh]

For the cflags to enable stack protectors to work properly they need to
be specified after -fno-stack-protector.

To do this fold all cflags into a single variable and move
-fno-stack-protector before the arch-specific cflags.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index c99bbcda7495..236c0364f5fb 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -77,8 +77,9 @@ Q=@
 endif
 
 CFLAGS_s390 = -m64
-CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables $(CFLAGS_$(ARCH))
-CFLAGS  += $(call cc-option,-fno-stack-protector)
+CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
+		$(call cc-option,-fno-stack-protector) \
+		$(CFLAGS_$(ARCH))
 LDFLAGS := -s
 
 help:

-- 
2.40.0

[PATCH v2 6/8] tools/nolibc: tests: add test for -fstack-protector

[Thomas Weißschuh]

Test the previously introduce stack protector functionality in nolibc.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/Makefile      |  3 ++
 tools/testing/selftests/nolibc/nolibc-test.c | 62 +++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 236c0364f5fb..b4f818547f35 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -76,6 +76,9 @@ else
 Q=@
 endif
 
+CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
+			$(call cc-option,-mstack-protector-guard=global) \
+			$(call cc-option,-fstack-protector-all)
 CFLAGS_s390 = -m64
 CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
 		$(call cc-option,-fno-stack-protector) \
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index fb2d4872fac9..21bacc928bf7 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -667,6 +667,63 @@ int run_stdlib(int min, int max)
 	return ret;
 }
 
+#if defined(__clang__)
+__attribute__((optnone))
+#elif defined(__GNUC__)
+__attribute__((optimize("O0")))
+#endif
+static int smash_stack(void)
+{
+	char buf[100];
+
+	for (size_t i = 0; i < 200; i++)
+		buf[i] = 'P';
+
+	return 1;
+}
+
+static int run_protection(int min, int max)
+{
+	pid_t pid;
+	int llen = 0, status;
+
+	llen += printf("0 -fstackprotector ");
+
+#if !defined(NOLIBC_STACKPROTECTOR)
+	llen += printf("not supported");
+	pad_spc(llen, 64, "[SKIPPED]\n");
+	return 0;
+#endif
+
+	pid = -1;
+	pid = fork();
+
+	switch (pid) {
+	case -1:
+		llen += printf("fork()");
+		pad_spc(llen, 64, "[FAIL]\n");
+		return 1;
+
+	case 0:
+		close(STDOUT_FILENO);
+		close(STDERR_FILENO);
+
+		smash_stack();
+		return 1;
+
+	default:
+		pid = waitpid(pid, &status, 0);
+
+		if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) {
+			llen += printf("waitpid()");
+			pad_spc(llen, 64, "[FAIL]\n");
+			return 1;
+		}
+		pad_spc(llen, 64, " [OK]\n");
+		return 0;
+	}
+}
+
 /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
 int prepare(void)
 {
@@ -719,8 +776,9 @@ int prepare(void)
 /* This is the definition of known test names, with their functions */
 static const struct test test_names[] = {
 	/* add new tests here */
-	{ .name = "syscall",   .func = run_syscall  },
-	{ .name = "stdlib",    .func = run_stdlib   },
+	{ .name = "syscall",    .func = run_syscall    },
+	{ .name = "stdlib",     .func = run_stdlib     },
+	{ .name = "protection", .func = run_protection },
 	{ 0 }
 };
 

-- 
2.40.0

[PATCH v2 7/8] tools/nolibc: i386: add stackprotector support

[Thomas Weißschuh]

Enable the new stackprotector support for i386.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/arch-i386.h        | 7 ++++++-
 tools/testing/selftests/nolibc/Makefile | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/include/nolibc/arch-i386.h b/tools/include/nolibc/arch-i386.h
index e8d0cf545bf1..2d98d78fd3f3 100644
--- a/tools/include/nolibc/arch-i386.h
+++ b/tools/include/nolibc/arch-i386.h
@@ -181,6 +181,8 @@ struct sys_stat_struct {
 char **environ __attribute__((weak));
 const unsigned long *_auxv __attribute__((weak));
 
+#define __ARCH_SUPPORTS_STACK_PROTECTOR
+
 /* startup code */
 /*
  * i386 System V ABI mandates:
@@ -188,9 +190,12 @@ const unsigned long *_auxv __attribute__((weak));
  * 2) The deepest stack frame should be set to zero
  *
  */
-void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void)
+void __attribute__((weak,noreturn,optimize("omit-frame-pointer"),no_stack_protector)) _start(void)
 {
 	__asm__ volatile (
+#ifdef NOLIBC_STACKPROTECTOR
+		"call __stack_chk_init\n"   // initialize stack protector
+#endif
 		"pop %eax\n"                // argc   (first arg, %eax)
 		"mov %esp, %ebx\n"          // argv[] (second arg, %ebx)
 		"lea 4(%ebx,%eax,4),%ecx\n" // then a NULL then envp (third arg, %ecx)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index b4f818547f35..8f069ebdd124 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -79,6 +79,7 @@ endif
 CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
 			$(call cc-option,-mstack-protector-guard=global) \
 			$(call cc-option,-fstack-protector-all)
+CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
 CFLAGS_s390 = -m64
 CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
 		$(call cc-option,-fno-stack-protector) \

-- 
2.40.0

[PATCH v2 8/8] tools/nolibc: x86_64: add stackprotector support

[Thomas Weißschuh]

Enable the new stackprotector support for x86_64.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/arch-x86_64.h      | 5 +++++
 tools/testing/selftests/nolibc/Makefile | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/tools/include/nolibc/arch-x86_64.h b/tools/include/nolibc/arch-x86_64.h
index 17f6751208e7..f7f2a11d4c3b 100644
--- a/tools/include/nolibc/arch-x86_64.h
+++ b/tools/include/nolibc/arch-x86_64.h
@@ -181,6 +181,8 @@ struct sys_stat_struct {
 char **environ __attribute__((weak));
 const unsigned long *_auxv __attribute__((weak));
 
+#define __ARCH_SUPPORTS_STACK_PROTECTOR
+
 /* startup code */
 /*
  * x86-64 System V ABI mandates:
@@ -191,6 +193,9 @@ const unsigned long *_auxv __attribute__((weak));
 void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void)
 {
 	__asm__ volatile (
+#ifdef NOLIBC_STACKPROTECTOR
+		"call __stack_chk_init\n"   // initialize stack protector
+#endif
 		"pop %rdi\n"                // argc   (first arg, %rdi)
 		"mov %rsp, %rsi\n"          // argv[] (second arg, %rsi)
 		"lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 8f069ebdd124..543555f4cbdc 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -80,6 +80,8 @@ CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
 			$(call cc-option,-mstack-protector-guard=global) \
 			$(call cc-option,-fstack-protector-all)
 CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
+CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
+CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
 CFLAGS_s390 = -m64
 CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
 		$(call cc-option,-fno-stack-protector) \

-- 
2.40.0

Re: [PATCH v2 0/8] tools/nolibc: add support for stack protector

[Willy Tarreau]

Hi Thomas,

On Mon, Mar 20, 2023 at 03:41:00PM +0000, Thomas Weißschuh wrote:
> This is useful when using nolibc for security-critical tools.
> Using nolibc has the advantage that the code is easily auditable and
> sandboxable with seccomp as no unexpected syscalls are used.
> Using compiler-assistent stack protection provides another security
> mechanism.
(...)

Thanks for this. I had a quick look over the patches and at first glance
it looks OK. I'll give it a try before this week-end on all supported
archs to rule out any potential side effect, and will queue it.

cheers,
Willy

Re: [PATCH v2 8/8] tools/nolibc: x86_64: add stackprotector support

[Willy Tarreau]

Hi Thomas,

On Mon, Mar 20, 2023 at 03:41:08PM +0000, Thomas Weißschuh wrote:
> Enable the new stackprotector support for x86_64.
(...)
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index 8f069ebdd124..543555f4cbdc 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -80,6 +80,8 @@ CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
>  			$(call cc-option,-mstack-protector-guard=global) \
>  			$(call cc-option,-fstack-protector-all)
>  CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
> +CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
> +CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
>  CFLAGS_s390 = -m64
>  CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
>  		$(call cc-option,-fno-stack-protector) \

This change is making it almost impossible for me to pass external CFLAGS
without forcefully disabling the automatic detection of stackprot. I need
to do it for some archs (e.g. "-march=armv5t -mthumb") or even to change
optimization levels.

I figured that the simplest way to recover that functionality for me
consists in using a dedicated variable to assign stack protector per
supported architecure and concatenating it to the per-arch CFLAGS like
this:

  diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
  index 543555f4cbdc..bbce57420465 100644
  --- a/tools/testing/selftests/nolibc/Makefile
  +++ b/tools/testing/selftests/nolibc/Makefile
  @@ -79,13 +79,13 @@ endif
   CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
                          $(call cc-option,-mstack-protector-guard=global) \
                          $(call cc-option,-fstack-protector-all)
  -CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
  -CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
  -CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
  +CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
  +CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
  +CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
   CFLAGS_s390 = -m64
   CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
                  $(call cc-option,-fno-stack-protector) \
  -               $(CFLAGS_$(ARCH))
  +               $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
   LDFLAGS := -s
   
   help:

And now with this it works again for me on all archs, with all of them
showing "SKIPPED" for the -fstackprotector line except i386/x86_64 which
show "OK".

Are you OK with this approach ? And if so, do you want to respin it or
do you want me to retrofit it into your 3 patches that introduce this
change (it's easy enough so I really don't care) ?

Thanks!
Willy

Re: [PATCH v2 8/8] tools/nolibc: x86_64: add stackprotector support

[Thomas Weißschuh]

Hi Willy,

On Thu, Mar 23, 2023 at 09:19:48PM +0100, Willy Tarreau wrote:
> On Mon, Mar 20, 2023 at 03:41:08PM +0000, Thomas Weißschuh wrote:
> > Enable the new stackprotector support for x86_64.
> (...)
> > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > index 8f069ebdd124..543555f4cbdc 100644
> > --- a/tools/testing/selftests/nolibc/Makefile
> > +++ b/tools/testing/selftests/nolibc/Makefile
> > @@ -80,6 +80,8 @@ CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> >  			$(call cc-option,-mstack-protector-guard=global) \
> >  			$(call cc-option,-fstack-protector-all)
> >  CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
> > +CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
> > +CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
> >  CFLAGS_s390 = -m64
> >  CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
> >  		$(call cc-option,-fno-stack-protector) \
> 
> This change is making it almost impossible for me to pass external CFLAGS
> without forcefully disabling the automatic detection of stackprot. I need
> to do it for some archs (e.g. "-march=armv5t -mthumb") or even to change
> optimization levels.
> 
> I figured that the simplest way to recover that functionality for me
> consists in using a dedicated variable to assign stack protector per
> supported architecure and concatenating it to the per-arch CFLAGS like
> this:
> 
>   diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
>   index 543555f4cbdc..bbce57420465 100644
>   --- a/tools/testing/selftests/nolibc/Makefile
>   +++ b/tools/testing/selftests/nolibc/Makefile
>   @@ -79,13 +79,13 @@ endif
>    CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
>                           $(call cc-option,-mstack-protector-guard=global) \
>                           $(call cc-option,-fstack-protector-all)
>   -CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
>   -CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
>   -CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
>   +CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
>   +CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
>   +CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
>    CFLAGS_s390 = -m64
>    CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
>                   $(call cc-option,-fno-stack-protector) \
>   -               $(CFLAGS_$(ARCH))
>   +               $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
>    LDFLAGS := -s
>    
>    help:
> 
> And now with this it works again for me on all archs, with all of them
> showing "SKIPPED" for the -fstackprotector line except i386/x86_64 which
> show "OK".
> 
> Are you OK with this approach ? And if so, do you want to respin it or
> do you want me to retrofit it into your 3 patches that introduce this
> change (it's easy enough so I really don't care) ?

Looks good to me.

If nothing else needs to be changed feel free to fix it up on your side.

Thanks,
Thomas

Re: [PATCH v2 8/8] tools/nolibc: x86_64: add stackprotector support

[Willy Tarreau]

On Thu, Mar 23, 2023 at 11:44:15PM +0000, Thomas Weißschuh wrote:
> Hi Willy,
> 
> On Thu, Mar 23, 2023 at 09:19:48PM +0100, Willy Tarreau wrote:
> > On Mon, Mar 20, 2023 at 03:41:08PM +0000, Thomas Weißschuh wrote:
> > > Enable the new stackprotector support for x86_64.
> > (...)
> > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > > index 8f069ebdd124..543555f4cbdc 100644
> > > --- a/tools/testing/selftests/nolibc/Makefile
> > > +++ b/tools/testing/selftests/nolibc/Makefile
> > > @@ -80,6 +80,8 @@ CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> > >  			$(call cc-option,-mstack-protector-guard=global) \
> > >  			$(call cc-option,-fstack-protector-all)
> > >  CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
> > > +CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
> > > +CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
> > >  CFLAGS_s390 = -m64
> > >  CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
> > >  		$(call cc-option,-fno-stack-protector) \
> > 
> > This change is making it almost impossible for me to pass external CFLAGS
> > without forcefully disabling the automatic detection of stackprot. I need
> > to do it for some archs (e.g. "-march=armv5t -mthumb") or even to change
> > optimization levels.
> > 
> > I figured that the simplest way to recover that functionality for me
> > consists in using a dedicated variable to assign stack protector per
> > supported architecure and concatenating it to the per-arch CFLAGS like
> > this:
> > 
> >   diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> >   index 543555f4cbdc..bbce57420465 100644
> >   --- a/tools/testing/selftests/nolibc/Makefile
> >   +++ b/tools/testing/selftests/nolibc/Makefile
> >   @@ -79,13 +79,13 @@ endif
> >    CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> >                           $(call cc-option,-mstack-protector-guard=global) \
> >                           $(call cc-option,-fstack-protector-all)
> >   -CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR)
> >   -CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR)
> >   -CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR)
> >   +CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
> >   +CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
> >   +CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
> >    CFLAGS_s390 = -m64
> >    CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables \
> >                   $(call cc-option,-fno-stack-protector) \
> >   -               $(CFLAGS_$(ARCH))
> >   +               $(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
> >    LDFLAGS := -s
> >    
> >    help:
> > 
> > And now with this it works again for me on all archs, with all of them
> > showing "SKIPPED" for the -fstackprotector line except i386/x86_64 which
> > show "OK".
> > 
> > Are you OK with this approach ? And if so, do you want to respin it or
> > do you want me to retrofit it into your 3 patches that introduce this
> > change (it's easy enough so I really don't care) ?
> 
> Looks good to me.
> 
> If nothing else needs to be changed feel free to fix it up on your side.

Perfect, will do it then. Thanks!
Willy