[LTP] [PATCH] security/stack_clash: Add test for mmap() minding gap

[Edgecombe, Rick P]

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.