| Message ID | 20230524202516.2190903-1-rick.p.edgecombe@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | security/stack_clash: Add test for mmap() minding gap | expand |
Hi! > 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 it be x86_64 for now, but document the assumptions so > that others can be added after more verification. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > > Hi, > > There was recently a regression around mmap placement and mmap guard > gaps. Today the stack clash test only tests if the gap can expand too > close to an adjacent mapping, but not if it mappings can be placed in the > gap. Can we enhance the test to also verifiy the latter? Is there an upstream commit fix? If so it should be put into the tags array. > testcases/cve/stack_clash.c | 95 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/testcases/cve/stack_clash.c b/testcases/cve/stack_clash.c > index cd7f148c2..bbd28b4f1 100644 > --- a/testcases/cve/stack_clash.c > +++ b/testcases/cve/stack_clash.c > @@ -18,6 +18,10 @@ > * 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 (See not above > + * CAN_DO_PLACEMENT_TEST), then the test also verifies that new mmap()s can't > + * be placed in the stack's guard gap. > + * > * [1] https://blog.qualys.com/securitylabs/2017/06/19/the-stack-clash > * [2] https://bugzilla.novell.com/show_bug.cgi?id=CVE-2017-1000364 > */ > @@ -45,6 +49,22 @@ static int STACK_GROWSDOWN; > > #define EXIT_TESTBROKE TBROK > > +/* > + * The mmap placement 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 > + * > + * Case 1 has been verified on x86_64, so only do the test on this > + * architecture until more have been verified. > + */ > +#ifdef __x86_64__ > +#define CAN_DO_PLACEMENT_TEST 1 > +#else > +#define CAN_DO_PLACEMENT_TEST 0 > +#endif > + > void exhaust_stack_into_sigsegv(void) > { > volatile char * ptr = alloca(FRAME_SIZE - sizeof(long)); > @@ -78,6 +98,57 @@ 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; > + > + fh = SAFE_FOPEN("/proc/sys/vm/mmap_min_addr", "r"); > + > + /* start filling from mmap_min_addr */ > + if (fscanf(fh, "%lu", &lastend) != 1) { > + SAFE_FCLOSE(fh); > + tst_brk(TBROK | TERRNO, "fscanf"); > + return; > + } > + > + SAFE_FCLOSE(fh); We do have SAFE_FILE_SCANF() as well. > + 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 +201,26 @@ 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(); > + > + /* > + * fill_gaps_with_prot_none() 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) > + _exit(EXIT_FAILURE); It would be better if we reported this as a separate result, since otherwise it wouldn't be clear if this part failed or if we got the EXIT_FAILURE from the SIGSEGV signal handler. As a matter of fact we can just do tst_res(TFAIL, "..."): here instead of exit, which will print a message, increment failure counter, and proccedd with the rest of the test. I suppose that we want to unmap this mapping just in case if we do so. > +} > + > void do_child(void) > { > unsigned long stack_addr, stack_size; > @@ -179,6 +270,10 @@ void do_child(void) > dump_proc_self_maps(); > #endif > > + if (CAN_DO_PLACEMENT_TEST) > + do_mmap_placement_test(stack_addr, gap); I do not think that the macro indirection does add any value, can we just do the #ifdef for x86 here? > + /* Now see if it can grow too close to an adjacent region. */ > exhaust_stack_into_sigsegv(); > } Otherwise it looks fine.
On Thu, 2023-05-25 at 10:53 +0200, Cyril Hrubis wrote: > Hi! > > 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 it be x86_64 for now, but document the > > assumptions so > > that others can be added after more verification. > > > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > > > > Hi, > > > > There was recently a regression around mmap placement and mmap > > guard > > gaps. Today the stack clash test only tests if the gap can expand > > too > > close to an adjacent mapping, but not if it mappings can be placed > > in the > > gap. Can we enhance the test to also verifiy the latter? > > Is there an upstream commit fix? If so it should be put into the tags > array. Oh, I see. Yes I can add it. > > > testcases/cve/stack_clash.c | 95 > > +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 95 insertions(+) > > > > diff --git a/testcases/cve/stack_clash.c > > b/testcases/cve/stack_clash.c > > index cd7f148c2..bbd28b4f1 100644 > > --- a/testcases/cve/stack_clash.c > > +++ b/testcases/cve/stack_clash.c > > @@ -18,6 +18,10 @@ > > * 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 (See not above > > + * CAN_DO_PLACEMENT_TEST), then the test also verifies that new > > mmap()s can't > > + * be placed in the stack's guard gap. > > + * > > * [1] > > https://blog.qualys.com/securitylabs/2017/06/19/the-stack-clash > > * [2] > > https://bugzilla.novell.com/show_bug.cgi?id=CVE-2017-1000364 > > */ > > @@ -45,6 +49,22 @@ static int STACK_GROWSDOWN; > > > > #define EXIT_TESTBROKE TBROK > > > > +/* > > + * The mmap placement 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 > > + * > > + * Case 1 has been verified on x86_64, so only do the test on this > > + * architecture until more have been verified. > > + */ > > +#ifdef __x86_64__ > > +#define CAN_DO_PLACEMENT_TEST 1 > > +#else > > +#define CAN_DO_PLACEMENT_TEST 0 > > +#endif > > + > > void exhaust_stack_into_sigsegv(void) > > { > > volatile char * ptr = alloca(FRAME_SIZE - sizeof(long)); > > @@ -78,6 +98,57 @@ 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; > > + > > + fh = SAFE_FOPEN("/proc/sys/vm/mmap_min_addr", "r"); > > + > > + /* start filling from mmap_min_addr */ > > + if (fscanf(fh, "%lu", &lastend) != 1) { > > + SAFE_FCLOSE(fh); > > + tst_brk(TBROK | TERRNO, "fscanf"); > > + return; > > + } > > + > > + SAFE_FCLOSE(fh); > > We do have SAFE_FILE_SCANF() as well. Oh, neat. I'll change it. > > > + 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 +201,26 @@ 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(); > > + > > + /* > > + * fill_gaps_with_prot_none() 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) > > + _exit(EXIT_FAILURE); > > It would be better if we reported this as a separate result, since > otherwise it wouldn't be clear if this part failed or if we got the > EXIT_FAILURE from the SIGSEGV signal handler. > > As a matter of fact we can just do tst_res(TFAIL, "..."): here > instead > of exit, which will print a message, increment failure counter, and > proccedd with the rest of the test. Sounds easy enough. > I suppose that we want to unmap this > mapping just in case if we do so. Right. The growing test actually will work with something in the gap, but it's more consistent to unmap it. > > > +} > > + > > void do_child(void) > > { > > unsigned long stack_addr, stack_size; > > @@ -179,6 +270,10 @@ void do_child(void) > > dump_proc_self_maps(); > > #endif > > > > + if (CAN_DO_PLACEMENT_TEST) > > + do_mmap_placement_test(stack_addr, gap); > > I do not think that the macro indirection does add any value, can we > just do the #ifdef for x86 here? Did you catch that this test likely will work for other architectures, but it just hasn't been verified? I was imagining having it this way might entice people to add them. Versus looking like some hopelessly x86 specific thing... > > > + /* Now see if it can grow too close to an adjacent region. > > */ > > exhaust_stack_into_sigsegv(); > > } > > Otherwise it looks fine.
diff --git a/testcases/cve/stack_clash.c b/testcases/cve/stack_clash.c index cd7f148c2..bbd28b4f1 100644 --- a/testcases/cve/stack_clash.c +++ b/testcases/cve/stack_clash.c @@ -18,6 +18,10 @@ * 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 (See not above + * CAN_DO_PLACEMENT_TEST), then the test also verifies that new mmap()s can't + * be placed in the stack's guard gap. + * * [1] https://blog.qualys.com/securitylabs/2017/06/19/the-stack-clash * [2] https://bugzilla.novell.com/show_bug.cgi?id=CVE-2017-1000364 */ @@ -45,6 +49,22 @@ static int STACK_GROWSDOWN; #define EXIT_TESTBROKE TBROK +/* + * The mmap placement 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 + * + * Case 1 has been verified on x86_64, so only do the test on this + * architecture until more have been verified. + */ +#ifdef __x86_64__ +#define CAN_DO_PLACEMENT_TEST 1 +#else +#define CAN_DO_PLACEMENT_TEST 0 +#endif + void exhaust_stack_into_sigsegv(void) { volatile char * ptr = alloca(FRAME_SIZE - sizeof(long)); @@ -78,6 +98,57 @@ 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; + + fh = SAFE_FOPEN("/proc/sys/vm/mmap_min_addr", "r"); + + /* start filling from mmap_min_addr */ + if (fscanf(fh, "%lu", &lastend) != 1) { + SAFE_FCLOSE(fh); + tst_brk(TBROK | TERRNO, "fscanf"); + return; + } + + SAFE_FCLOSE(fh); + + 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 +201,26 @@ 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(); + + /* + * fill_gaps_with_prot_none() 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) + _exit(EXIT_FAILURE); +} + void do_child(void) { unsigned long stack_addr, stack_size; @@ -179,6 +270,10 @@ void do_child(void) dump_proc_self_maps(); #endif + if (CAN_DO_PLACEMENT_TEST) + do_mmap_placement_test(stack_addr, gap); + + /* Now see if it can grow too close to an adjacent region. */ exhaust_stack_into_sigsegv(); }
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 it be x86_64 for now, but document the assumptions so that others can be added after more verification. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- Hi, There was recently a regression around mmap placement and mmap guard gaps. Today the stack clash test only tests if the gap can expand too close to an adjacent mapping, but not if it mappings can be placed in the gap. Can we enhance the test to also verifiy the latter? Thanks, Rick testcases/cve/stack_clash.c | 95 +++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)