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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202408051025.33CA44DEE6@keescook>
Date: Mon, 5 Aug 2024 10:26:36 -0700
From: Kees Cook <kees@...nel.org>
To: David Gow <davidgow@...gle.com>
Cc: Alexandre Ghiti <alex@...ti.fr>, Luis Chamberlain <mcgrof@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Mark Rutland <mark.rutland@....com>, linux-mm@...ck.org,
	linux-arm-kernel@...ts.infradead.org, kunit-dev@...glegroups.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: Only enforce minimum stack gap size if it's sensible

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@...gle.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@...nel.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