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

[Petr Vorel <pvorel@suse.cz>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp