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@xxxxxxxxxxxxxx/ > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx> > > --- > > 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.
