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

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

[David Gow]

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

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

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

[Kees Cook]

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