| Message ID | 20230711170335.13142-1-rick.p.edgecombe@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] security/stack_clash: Add test for mmap() minding gap | expand |
Hi Rick, Reviewed-by: Petr Vorel <pvorel@suse.cz> ... > This logic is somewhat x86_64 specific, but may work for other > architectures. Make the test only run on x86_64 for now. ... > v3: > - Use lapi/mmap.h (Petr Vorel) > v2: > - Add fixes commit (Cyril Hrubis) > - Report placement test failure separately (Cyril Hrubis) > - Use SAFE_FILE_SCANF (Cyril Hrubis) > - Don't quit after failing placement test, so unmap the mapping that > caused the failure. (Cyril Hrubis) > - Drop CAN_DO_PLACEMENT_TEST (Cyril Hrubis) > --- > testcases/cve/stack_clash.c | 81 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 80 insertions(+), 1 deletion(-) nit: You might want to add your copyright. > diff --git a/testcases/cve/stack_clash.c b/testcases/cve/stack_clash.c > index cd7f148c2..50a401e96 100644 > --- a/testcases/cve/stack_clash.c > +++ b/testcases/cve/stack_clash.c > @@ -18,11 +18,18 @@ > * to infinity and preallocate REQ_STACK_SIZE bytes of stack so that no calls > * after `mmap` are moving stack further. > * > + * If the architecture meets certain requirements (only x86_64 is verified) > + * then the test also tests that new mmap()s can't be placed in the stack's > + * guard gap. This part of the test works by forcing a bottom up search. The > + * assumptions are that the stack grows down (start gap) and either: > + * 1. The default search is top down, and will switch to bottom up if > + * space is exhausted. > + * 2. The default search is bottom up and the stack is above mmap base > + * > * [1] https://blog.qualys.com/securitylabs/2017/06/19/the-stack-clash > * [2] https://bugzilla.novell.com/show_bug.cgi?id=CVE-2017-1000364 > */ Doc could be turned into our docparse format (to place text in our automatically generated documentation), but I can do it afterwards. ... > +static void do_mmap_placement_test(unsigned long stack_addr, unsigned long gap) > +{ > + void *map_test_gap; > + > + force_bottom_up(); > + > + /* > + * force_bottom_up() used up all the spaces below the stack. The search down > + * path should fail, and search up might take a look at the guard gap > + * region. If it avoids it, the allocation will be above the stack. If it > + * uses it, the allocation will be in the gap and the test should fail. > + */ > + map_test_gap = SAFE_MMAP(0, MAPPED_LEN, > + PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, 0, 0); > + > + if (stack_addr - gap <= (unsigned long)map_test_gap && > + (unsigned long)map_test_gap <= stack_addr) { > + tst_res(TFAIL, "New mmap was placed in the guard gap."); > + SAFE_MUNMAP(map_test_gap, MAPPED_LEN); > + } > +} > + > void do_child(void) > { > unsigned long stack_addr, stack_size; > @@ -179,6 +252,11 @@ void do_child(void) > dump_proc_self_maps(); > #endif > +#ifdef __x86_64__ I wonder whether we should consider 32 bit as well: #if defined(__x86_64__) || defined(__i386__) Or is it really just for 64 bit? > + do_mmap_placement_test(stack_addr, gap); > +#endif ... Kind regards, Petr
On Wed, 2023-07-12 at 10:09 +0200, Petr Vorel wrote: > Hi Rick, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> Thanks! > > ... > > This logic is somewhat x86_64 specific, but may work for other > > architectures. Make the test only run on x86_64 for now. > ... > > v3: > > - Use lapi/mmap.h (Petr Vorel) > > > v2: > > - Add fixes commit (Cyril Hrubis) > > - Report placement test failure separately (Cyril Hrubis) > > - Use SAFE_FILE_SCANF (Cyril Hrubis) > > - Don't quit after failing placement test, so unmap the mapping > > that > > caused the failure. (Cyril Hrubis) > > - Drop CAN_DO_PLACEMENT_TEST (Cyril Hrubis) > > --- > > testcases/cve/stack_clash.c | 81 > > ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 80 insertions(+), 1 deletion(-) > > nit: You might want to add your copyright. What do you mean? It's not a new file.. > > > diff --git a/testcases/cve/stack_clash.c > > b/testcases/cve/stack_clash.c > > index cd7f148c2..50a401e96 100644 > > --- a/testcases/cve/stack_clash.c > > +++ b/testcases/cve/stack_clash.c > > @@ -18,11 +18,18 @@ > > * to infinity and preallocate REQ_STACK_SIZE bytes of stack so > > that no calls > > * after `mmap` are moving stack further. > > * > > + * If the architecture meets certain requirements (only x86_64 is > > verified) > > + * then the test also tests that new mmap()s can't be placed in > > the stack's > > + * guard gap. This part of the test works by forcing a bottom up > > search. The > > + * assumptions are that the stack grows down (start gap) and > > either: > > + * 1. The default search is top down, and will switch to bottom > > up if > > + * space is exhausted. > > + * 2. The default search is bottom up and the stack is above > > mmap base > > + * > > * [1] > > https://blog.qualys.com/securitylabs/2017/06/19/the-stack-clash > > * [2] > > https://bugzilla.novell.com/show_bug.cgi?id=CVE-2017-1000364 > > */ > > Doc could be turned into our docparse format (to place text in our > automatically > generated documentation), but I can do it afterwards. Oh, sorry. I didn't know about it. > > ... > > > +static void do_mmap_placement_test(unsigned long stack_addr, > > unsigned long gap) > > +{ > > + void *map_test_gap; > > + > > + force_bottom_up(); > > + > > + /* > > + * force_bottom_up() used up all the spaces below the > > stack. The search down > > + * path should fail, and search up might take a look at the > > guard gap > > + * region. If it avoids it, the allocation will be above > > the stack. If it > > + * uses it, the allocation will be in the gap and the test > > should fail. > > + */ > > + map_test_gap = SAFE_MMAP(0, MAPPED_LEN, > > + PROT_READ|PROT_WRITE, > > MAP_ANON|MAP_PRIVATE, 0, 0); > > + > > + if (stack_addr - gap <= (unsigned long)map_test_gap && > > + (unsigned long)map_test_gap <= stack_addr) { > > + tst_res(TFAIL, "New mmap was placed in the guard > > gap."); > > + SAFE_MUNMAP(map_test_gap, MAPPED_LEN); > > + } > > +} > > + > > void do_child(void) > > { > > unsigned long stack_addr, stack_size; > > @@ -179,6 +252,11 @@ void do_child(void) > > dump_proc_self_maps(); > > #endif > > > +#ifdef __x86_64__ > I wonder whether we should consider 32 bit as well: > > #if defined(__x86_64__) || defined(__i386__) > > Or is it really just for 64 bit? It is probably for *most* architectures that implement top down mmap behavior. I think 32 bit x86 is not one of them. I had thought other architectures might enable this test, but I don't have an easy way to test them all. v1 actually had the enabling configuration separated out a little bit to try to entice people to enable other architectures. > > > + do_mmap_placement_test(stack_addr, gap); > > +#endif > ...
Hi! > > > ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > nit: You might want to add your copyright. > > What do you mean? It's not a new file.. Probably just to add "Copyright (c) 2023 your name" to the top of the C file
Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
On Thu, 2023-07-13 at 10:05 +0200, Cyril Hrubis wrote: > Hi! > > > > ++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > > > nit: You might want to add your copyright. > > > > What do you mean? It's not a new file.. > > Probably just to add "Copyright (c) 2023 your name" to the top of the > C file Should I send another version or do you guys want to just fixup these two things? (docparse and copyright)
Hi Rick, > On Thu, 2023-07-13 at 10:05 +0200, Cyril Hrubis wrote: > > Hi! > > > > > ++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > nit: You might want to add your copyright. > > > What do you mean? It's not a new file.. > > Probably just to add "Copyright (c) 2023 your name" to the top of the > > C file > Should I send another version or do you guys want to just fixup these > two things? (docparse and copyright) I added your copyright and merged. I'll fix docparse later today in a separate commit. Thank you! Kind regards, Petr
On Thu, 2023-07-20 at 17:47 +0200, Petr Vorel wrote: > Hi Rick, > > > On Thu, 2023-07-13 at 10:05 +0200, Cyril Hrubis wrote: > > > Hi! > > > > > > ++++++++++++++++++++++++++++++++++++- > > > > > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > > > nit: You might want to add your copyright. > > > > > What do you mean? It's not a new file.. > > > > Probably just to add "Copyright (c) 2023 your name" to the top of > > > the > > > C file > > > Should I send another version or do you guys want to just fixup > > these > > two things? (docparse and copyright) > > I added your copyright and merged. I'll fix docparse later today in a > separate > commit. Thank you! Thank you!
diff --git a/testcases/cve/stack_clash.c b/testcases/cve/stack_clash.c index cd7f148c2..50a401e96 100644 --- a/testcases/cve/stack_clash.c +++ b/testcases/cve/stack_clash.c @@ -18,11 +18,18 @@ * to infinity and preallocate REQ_STACK_SIZE bytes of stack so that no calls * after `mmap` are moving stack further. * + * If the architecture meets certain requirements (only x86_64 is verified) + * then the test also tests that new mmap()s can't be placed in the stack's + * guard gap. This part of the test works by forcing a bottom up search. The + * assumptions are that the stack grows down (start gap) and either: + * 1. The default search is top down, and will switch to bottom up if + * space is exhausted. + * 2. The default search is bottom up and the stack is above mmap base + * * [1] https://blog.qualys.com/securitylabs/2017/06/19/the-stack-clash * [2] https://bugzilla.novell.com/show_bug.cgi?id=CVE-2017-1000364 */ -#include <sys/mman.h> #include <sys/wait.h> #include <stdio.h> #include <unistd.h> @@ -32,6 +39,7 @@ #include "tst_test.h" #include "tst_safe_stdio.h" +#include "lapi/mmap.h" static unsigned long page_size; static unsigned long page_mask; @@ -78,6 +86,49 @@ void segv_handler(int sig, siginfo_t *info, void *data LTP_ATTRIBUTE_UNUSED) _exit(EXIT_SUCCESS); } +static void force_bottom_up(void) +{ + FILE *fh; + char buf[1024]; + unsigned long start, end, size, lastend = 0; + + /* start filling from mmap_min_addr */ + SAFE_FILE_SCANF("/proc/sys/vm/mmap_min_addr", "%lu", &lastend); + + fh = SAFE_FOPEN("/proc/self/maps", "r"); + + while (!feof(fh)) { + if (fgets(buf, sizeof(buf), fh) == NULL) + goto out; + + if (sscanf(buf, "%lx-%lx", &start, &end) != 2) { + tst_brk(TBROK | TERRNO, "sscanf"); + goto out; + } + + size = start - lastend; + + /* Skip the PROT_NONE that was just added (!size). */ + if (!size) { + lastend = end; + continue; + } + + /* If the next area is the stack, quit. */ + if (!!strstr(buf, "[stack]")) + break; + + /* This is not cleaned up. */ + SAFE_MMAP((void *)lastend, size, PROT_NONE, + MAP_ANON|MAP_PRIVATE|MAP_FIXED_NOREPLACE, -1, 0); + + lastend = end; + } + +out: + SAFE_FCLOSE(fh); +} + unsigned long read_stack_addr_from_proc(unsigned long *stack_size) { FILE *fh; @@ -130,6 +181,28 @@ void __attribute__((noinline)) preallocate_stack(unsigned long required) garbage[0] = garbage[required - 1] = '\0'; } +static void do_mmap_placement_test(unsigned long stack_addr, unsigned long gap) +{ + void *map_test_gap; + + force_bottom_up(); + + /* + * force_bottom_up() used up all the spaces below the stack. The search down + * path should fail, and search up might take a look at the guard gap + * region. If it avoids it, the allocation will be above the stack. If it + * uses it, the allocation will be in the gap and the test should fail. + */ + map_test_gap = SAFE_MMAP(0, MAPPED_LEN, + PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, 0, 0); + + if (stack_addr - gap <= (unsigned long)map_test_gap && + (unsigned long)map_test_gap <= stack_addr) { + tst_res(TFAIL, "New mmap was placed in the guard gap."); + SAFE_MUNMAP(map_test_gap, MAPPED_LEN); + } +} + void do_child(void) { unsigned long stack_addr, stack_size; @@ -179,6 +252,11 @@ void do_child(void) dump_proc_self_maps(); #endif +#ifdef __x86_64__ + do_mmap_placement_test(stack_addr, gap); +#endif + + /* Now see if it can grow too close to an adjacent region. */ exhaust_stack_into_sigsegv(); } @@ -252,6 +330,7 @@ static struct tst_test test = { .test_all = stack_clash_test, .tags = (const struct tst_tag[]) { {"CVE", "2017-1000364"}, + {"linux-git", "58c5d0d6d522"}, {} } };
The existing stack_clash test only verifies if the stack can grow too close to an existing mapping. It doesn't test if mmap() will place new mappings in the gap. Add a test for this. Have it fill all the empty regions below the stack with PROT_NONE mappings. Do this by searching /proc/pid/maps for the gaps. The code for parsing this is based on the existing read_stack_addr_from_proc() in the file. With all lower spaces taken by the PROT_NONE mappings, the search down path will then fail for new mmap()s. So mmap() will search up and find the gap just before the stack. If it picks it then the mapping is in the guard region, so fail the test. This logic is somewhat x86_64 specific, but may work for other architectures. Make the test only run on x86_64 for now. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v3: - Use lapi/mmap.h (Petr Vorel) v2: - Add fixes commit (Cyril Hrubis) - Report placement test failure separately (Cyril Hrubis) - Use SAFE_FILE_SCANF (Cyril Hrubis) - Don't quit after failing placement test, so unmap the mapping that caused the failure. (Cyril Hrubis) - Drop CAN_DO_PLACEMENT_TEST (Cyril Hrubis) --- testcases/cve/stack_clash.c | 81 ++++++++++++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-)