| Message ID | 20230223-nolibc-stackprotector-v1-5-3e74d81b3f21@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: add support for stack protector | expand |
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). > 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. > }; > > #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. > +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. > + > + 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)). > + 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. > + 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. > + 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. Thanks, Willy
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.
On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Weißschuh wrote: > 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/ Ah thanks, it seems I indeed missed it. It looks good, I'll take it. > > > +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. Ah indeed makes sense. > The overhead of doing this should be neglectable. It was not the overhead (that's only a regtest program after all), I was more thinking about the difficulty to maintain this function over time for other archs if it starts to rely on optional support. But for now it's not a problem, it it would ever become one we could simply change that to have a function just print SKIPPED. So I'm fine with your option. > > > @@ -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. Ah I didn't realize that, I now understand how that can be useful, indeed. Then maybe just rename your .skip_by_default field to .hidden so that it becomes more generic (i.e. if one day we permit enumeration we don't want such tests to be listed either), and assign the field on the same line so that it's easily visible with a grep. Thanks, Willy
On Mon, Mar 13, 2023 at 04:08:10AM +0100, Willy Tarreau wrote: > On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Weißschuh wrote: > > 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/ > > Ah thanks, it seems I indeed missed it. It looks good, I'll take it. Do you have a tree with this published? So I can make sure the next revision of this patchset does not lead to conflicts. > > > > +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. > > Ah indeed makes sense. > > > The overhead of doing this should be neglectable. > > It was not the overhead (that's only a regtest program after all), I > was more thinking about the difficulty to maintain this function over > time for other archs if it starts to rely on optional support. But for > now it's not a problem, it it would ever become one we could simply > change that to have a function just print SKIPPED. So I'm fine with > your option. > > > > > @@ -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. > > Ah I didn't realize that, I now understand how that can be useful, > indeed. Then maybe just rename your .skip_by_default field to .hidden > so that it becomes more generic (i.e. if one day we permit enumeration > we don't want such tests to be listed either), and assign the field on > the same line so that it's easily visible with a grep. Actually this works fine with a plain fork() and the exec() is not needed. So the dedicated entrypoint is not needed anymore. No idea what I tested before.
Hi Thomas, On Sat, Mar 18, 2023 at 04:49:12PM +0000, Thomas Weißschuh wrote: > On Mon, Mar 13, 2023 at 04:08:10AM +0100, Willy Tarreau wrote: > > On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Weißschuh wrote: > > > 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/ > > > > Ah thanks, it seems I indeed missed it. It looks good, I'll take it. > > Do you have a tree with this published? No, it was only on my local machine waiting for me to retest all archs with it (in the past I've met build issues due to some variables being preset by some included files so I'm extra careful). I've just rebased it on latest master and just passed it to Paul for inclusion now. > So I can make sure the next revision of this patchset does not lead to > conflicts. Do not worry too much for this, just tell me upfront whether your next series is based on it or not and I'll adjust accordingly based on what is already merged when I take it. > > > > > @@ -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. > > > > Ah I didn't realize that, I now understand how that can be useful, > > indeed. Then maybe just rename your .skip_by_default field to .hidden > > so that it becomes more generic (i.e. if one day we permit enumeration > > we don't want such tests to be listed either), and assign the field on > > the same line so that it's easily visible with a grep. > > Actually this works fine with a plain fork() and the exec() is not > needed. So the dedicated entrypoint is not needed anymore. Ah, even better! > No idea what I tested before. No worries. I've yet to find a single occurrence of a test being created straight without exploring various approaches ;-) Thanks! Willy
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(-)