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

[Edgecombe, Rick P]

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
> ...