LinuxLists.cc - [PATCH v2 0/8] tools/nolibc: add support for stack protector

2023-03-20 15:51:03

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

2023-03-20 15:51:06

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

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

Signed-off-by: Thomas Weißschuh <[email protected]>
---
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

2023-03-20 15:51:09

by Thomas Weißschuh

[permalink] [raw]

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

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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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

2023-03-20 15:51:12

by Thomas Weißschuh

[permalink] [raw]

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

Nothing ever modifies this structure.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
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

2023-03-20 15:51:19

by Thomas Weißschuh

[permalink] [raw]

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

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

Signed-off-by: Thomas Weißschuh <[email protected]>
---
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

2023-03-20 15:51:25

by Thomas Weißschuh

[permalink] [raw]

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

Test the previously introduce stack protector functionality in nolibc.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
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

2023-03-20 15:51:29

by Thomas Weißschuh

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-03-20 15:51:32

by Thomas Weißschuh

[permalink] [raw]

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

Enable the new stackprotector support for i386.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
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

2023-03-20 15:51:35

by Thomas Weißschuh

[permalink] [raw]

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

Enable the new stackprotector support for x86_64.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
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

2023-03-20 22:06:50

by Willy Tarreau

[permalink] [raw]

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

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

2023-03-23 20:41:03

by Willy Tarreau

[permalink] [raw]

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

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

2023-03-23 23:45:26

by Thomas Weißschuh

[permalink] [raw]

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

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

2023-03-24 06:08:08

by Willy Tarreau

[permalink] [raw]

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

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