| Message ID | 20230223-nolibc-stackprotector-v1-5-3e74d81b3f21@weissschuh.net |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: add support for stack protector | expand |
On Sun, Mar 12, 2023 at 02:07:16PM +0100, Willy Tarreau wrote: > On Tue, Mar 07, 2023 at 10:22:34PM +0000, Thomas Weißschuh wrote: > > Test the previously introduce stack protector functionality in nolibc. > > s/introduce/introduced/ > > (I can adjust it myself when merging to avoid a respin if you want). I respin is necessary anways. I'll change it. FYI there is also another patch to make nolibc-test buildable with compilers that enable -fstack-protector by default. Maybe this can be picked up until the proper stack-protector support is hashed out. Maybe even for 6.3: https://lore.kernel.org/lkml/20230221-nolibc-no-stack-protector-v1-1-4e6a42f969e2@weissschuh.net/ > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++++++++++++++++++- > > 1 file changed, 72 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > > index fb2d4872fac9..4990b2750279 100644 > > --- a/tools/testing/selftests/nolibc/nolibc-test.c > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > > @@ -45,6 +45,7 @@ char **environ; > > struct test { > > const char *name; // test name > > int (*func)(int min, int max); // handler > > + char skip_by_default; // don't run by default > > Just a tiny detail but that comment is misaligned by one char on the left. Ack. > > }; > > > > #ifndef _NOLIBC_STDLIB_H > > @@ -667,6 +668,70 @@ int run_stdlib(int min, int max) > > return ret; > > } > > > > +#if defined(__clang__) > > +__attribute__((optnone)) > > +#elif defined(__GNUC__) > > +__attribute__((optimize("O0"))) > > +#endif > > +static int run_smash_stack(int min, int max) > > +{ > > + char buf[100]; > > + > > + for (size_t i = 0; i < 200; i++) > > + buf[i] = 15; > > If the goal is to make it easy to spot in a crash dump, I suggest > that you use a readable ASCII letter that's easy to recognize. 0xF > will usually not be printed in hex dumps, making it less evident > when scrolling quickly. For example I often use 'P' when poisoning > memory but you get the idea. Ack. > > +int run_stackprotector(int min, int max) > > +{ > > + int llen = 0; > > + > > + llen += printf("0 "); > > + > > +#if !defined(NOLIBC_STACKPROTECTOR) > > + llen += printf("stack smashing detection not supported"); > > + pad_spc(llen, 64, "[SKIPPED]\n"); > > + return 0; > > +#endif > > Shouldn't the whole function be enclosed instead ? I know it's more of > a matter of taste, but avoiding to build and link it for archs that > will not use it may be better. The goal was to print a [SKIPPED] message if it's not supported. The overhead of doing this should be neglectable. > > > + > > + pid_t pid = fork(); > > Please avoid variable declarations after statements, for me these > are really horrible to deal with when editing the code later, because > instead of having to look up only the beginning of each containing > block (i.e. in O(log(N))) you have to visually parse every single line > (i.e. O(N)). Ack. > > + switch (pid) { > > + case -1: > > + llen += printf("fork()"); > > + pad_spc(llen, 64, "[FAIL]\n"); > > + return 1; > > + > > + case 0: > > + close(STDOUT_FILENO); > > + close(STDERR_FILENO); > > + > > + char *const argv[] = { > > + "/proc/self/exe", > > + "_smash_stack", > > + NULL, > > + }; > > Same here. Ack. > > + execve("/proc/self/exe", argv, NULL); > > + return 1; > > + > > + default: { > > + int status; > > And here by moving "status" upper in the function you can even > get rid of the braces. Ack. > > + pid = waitpid(pid, &status, 0); > > + > > + if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) { > > + llen += printf("waitpid()"); > > + pad_spc(llen, 64, "[FAIL]\n"); > > + return 1; > > + } > > + llen += printf("stack smashing detected"); > > + 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 +784,11 @@ 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 = "stackprotector", .func = run_stackprotector, }, > > + { .name = "_smash_stack", .func = run_smash_stack, > > I think it would be better to keep the number of categories low > and probably you should add just one called "protection" or so, > and implement your various tests in it as is done for other > categories. The goal is to help developers quickly spot and select > the few activities they're interested in at a given moment. I'm not sure how this would be done. The goal here is that "stackprotector" is the user-visible category. It can be changed to "protection". "_smash_stack" however is just an entrypoint that is used by the forked process to call the crashing code. We need the fork+exec+special entrypoint to avoid crashing the test process itself.
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index fb2d4872fac9..4990b2750279 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -45,6 +45,7 @@ char **environ; struct test { const char *name; // test name int (*func)(int min, int max); // handler + char skip_by_default; // don't run by default }; #ifndef _NOLIBC_STDLIB_H @@ -667,6 +668,70 @@ int run_stdlib(int min, int max) return ret; } +#if defined(__clang__) +__attribute__((optnone)) +#elif defined(__GNUC__) +__attribute__((optimize("O0"))) +#endif +static int run_smash_stack(int min, int max) +{ + char buf[100]; + + for (size_t i = 0; i < 200; i++) + buf[i] = 15; + + return 1; +} + +int run_stackprotector(int min, int max) +{ + int llen = 0; + + llen += printf("0 "); + +#if !defined(NOLIBC_STACKPROTECTOR) + llen += printf("stack smashing detection not supported"); + pad_spc(llen, 64, "[SKIPPED]\n"); + return 0; +#endif + + pid_t 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); + + char *const argv[] = { + "/proc/self/exe", + "_smash_stack", + NULL, + }; + execve("/proc/self/exe", argv, NULL); + return 1; + + default: { + int status; + + pid = waitpid(pid, &status, 0); + + if (pid == -1 || !WIFSIGNALED(status) || WTERMSIG(status) != SIGABRT) { + llen += printf("waitpid()"); + pad_spc(llen, 64, "[FAIL]\n"); + return 1; + } + llen += printf("stack smashing detected"); + 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 +784,11 @@ 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 = "stackprotector", .func = run_stackprotector, }, + { .name = "_smash_stack", .func = run_smash_stack, + .skip_by_default = 1 }, { 0 } }; @@ -811,6 +879,8 @@ int main(int argc, char **argv, char **envp) } else { /* no test mentioned, run everything */ for (idx = 0; test_names[idx].name; idx++) { + if (test_names[idx].skip_by_default) + continue; printf("Running test '%s'\n", test_names[idx].name); err = test_names[idx].func(min, max); ret += err;
Test the previously introduce stack protector functionality in nolibc. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 2 deletions(-)