Re: [PATCH] mm: Only enforce minimum stack gap size if it's sensible

[Kees Cook <kees@kernel.org>]

On Sat, Aug 03, 2024 at 03:46:41PM +0800, David Gow wrote:
> The generic mmap_base code tries to leave a gap between the top
> of the stack and the mmap base address, but enforces a minimum
> gap size (MIN_GAP) of 128MB, which is too large on some setups. In
> particular, on arm tasks without ADDR_LIMIT_32BIT, the STACK_TOP
> value is less than 128MB, so it's impossible to fit such a gap in.
> 
> Only enforce this minimum if MIN_GAP < MAX_GAP, as we'd prefer to honour
> MAX_GAP, which is defined proportionally, so scales better and always
> leaves us with both _some_ stack space and some room for mmap.
> 
> This fixes the usercopy KUnit test suite on 32-bit arm, as it doesn't
> set any personality flags so gets the default (in this case 26-bit)
> task size. This test can be run with:
> ./tools/testing/kunit/kunit.py run --arch arm usercopy --make_options LLVM=1
> 
> Fixes: dba79c3df4a2 ("arm: use generic mmap top-down layout and brk randomization")
> Signed-off-by: David Gow <davidgow@google.com>
> ---
> 
> This is one possible fix for an issue with the usercopy_kunit suite
> (and, indeed, the KUnit user_alloc features) on 32-bit arm. The other
> options are to:
> - hack the KUnit allocation to force ADDR_LIMIT_32BIT or
>   ADDR_COMPAT_LAYOUT; or
> - similarly, use an unlimited stack, which forces the legacy layout
>   behind the scenes; or
> - adjust MIN_GAP based on either STACK_TOP or architecture.
> 
> Of them, I made the arbitrary call that this was least hacky, but am
> happy to go with something else if someone who actually knows what's
> going on suggests it.
> 
> (Also, does this issue actually mean some strange legacy binaries have
> been broken with an rlimit-ed stack for ages? Or am I missing something?)
> 
> Cheers,
> -- David

I see akpm already snagged this, but yeah, this looks like a totally
sane fix. Thanks for digging in and finding the problem!

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

> 
> ---
>  mm/util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index bd283e2132e0..baca6cafc9f1 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -463,7 +463,7 @@ static unsigned long mmap_base(unsigned long rnd, struct rlimit *rlim_stack)
>  	if (gap + pad > gap)
>  		gap += pad;
>  
> -	if (gap < MIN_GAP)
> +	if (gap < MIN_GAP && MIN_GAP < MAX_GAP)
>  		gap = MIN_GAP;
>  	else if (gap > MAX_GAP)
>  		gap = MAX_GAP;
> -- 
> 2.46.0.rc2.264.g509ed76dc8-goog
> 

-- 
Kees Cook