Message-ID: <1463478563.2555.27.camel@linaro.org>
Date:	Tue, 17 May 2016 10:49:23 +0100
From:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	Kees Cook <keescook@...omium.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Guenter Roeck <linux@...ck-us.net>,
	Arun Chandran <achandran@...sta.com>
Subject: Re: [PATCH 3.14 17/17] arm64: Make arch_randomize_brk avoid stack
 area

On Mon, 2016-05-16 at 18:14 -0700, Greg Kroah-Hartman wrote:
> 3.14-stable review patch.  If anyone has any objections, please let me know.

As reported by Guenter Roeck, this patch doesn't compile on 3.14 because
it deleted randomize_base which is still used by the macro
ELF_ET_DYN_BASE. That use was removed in 3.18 by commit 92980405f353
("arm64: ASLR: Don't randomise text when randomise_va_space == 0")

Looking at that commit it seems to be what caused the bug $subject patch
fixes because it stopped the arm64 implementation putting loaded
binaries 2/3rds the way up a task's address range.

So it seems to me, either $subject patch should only be applied to 
Linux versions 3.18 through 4.0 inclusive; or the fix in commit
92980405f353 also needs backporting to stable kernels before 3.18. (Or
some more other solution.)

Either way, it seems $subject patch also needs...

Fixes: 92980405f353 ("arm64: ASLR: Don't randomise text when randomise_va_space == 0")

Leaving patch quoted below for reference...
> 
> From: Jon Medhurst <tixy@...aro.org>
> 
> [As mentioned in the commit message, the problem this patch fixes can't
> occur in kernels with commit d1fd836dcf00, i.e Linux 4.1 and later.,
> but earlier kernel versions need this fix.]
> 
> When a process is created, various address randomisations could end up
> colluding to place the address of brk in the stack memory. This would
> mean processes making small heap based memory allocations are in danger
> of having them overwriting, or overwritten by, the stack.
> 
> Another consequence, is that even for processes that make no use of
> brk, the output of /proc/*/maps may show the stack area listed as
> '[heap]' rather than '[stack]'. Apart from being misleading this causes
> fatal errors with the Android run-time like:
> "No [stack] line found in /proc/self/task/*/maps"
> 
> To prevent this problem pick a limit for brk that allows for the stack's
> memory. At the same time we remove randomize_base() as that was only
> used by arch_randomize_brk().
> 
> Note, in practice, since commit d1fd836dcf00 ("mm: split ET_DYN ASLR
> from mmap ASLR") this problem shouldn't occur because the address chosen
> for loading binaries is well clear of the stack, however, prior to that
> the problem does occur because of the following...
> 
> The memory layout of a task is determined by arch_pick_mmap_layout. If
> address randomisation is enabled (task has flag PF_RANDOMIZE) this sets
> mmap_base to a random address at the top of task memory just below a
> region calculated to allow for a stack which itself may have a random
> base address. Any mmap operations that then happen which require an
> address allocating will use the topdown allocation method, i.e. the
> first allocated memory will be at the top of memory, just below the
> area set aside for the stack.
> 
> When a relocatable binary is loaded into a new process by
> load_elf_binary and randomised address are enabled, it uses a
> 'load_bias' of zero, so that when mmap is called to create a memory
> region for it, a new address is picked (address zero not being
> available). As this is the first memory region in the task, it gets the
> region just below the stack, as described previously.
> 
> The loader then set's brk to the end of the elf data section, which will
> be near the end of the loaded binary and then it calls
> arch_randomize_brk. As this currently stands, this adds a random amount
> to brk, which unfortunately may take it into the address range where the
> stack lies.
> 
> Testing:
> 
> These changes have been tested on Linux 3.18 (where the collision of brk
> and stack can happen) using 100000 invocations of a program [1] that can
> display the offset of a process's brk...
> 
> $for i in $(seq 100000); do ./aslr --report brk ; done
> 
> This shows values of brk are evenly distributed over a 1GB range before
> this change is applied. After this change the distribution shows a slope
> where lower values for brk are more common and upper values have about
> half the frequency of those.
> 
> [1] http://bazaar.launchpad.net/~ubuntu-bugcontrol/qa-regression-testing/master/files/2499/scripts/kernel-security/aslr/
> 
> Signed-off-by: Jon Medhurst <tixy@...aro.org>
> Acked-by: Kees Cook <keescook@...omium.org>
> Acked-by: Catalin Marinas <catalin.marinas@....com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
> 
> 
> I originally posted this to the ARM kernel list and arm64 maintainers,
> see http://www.spinics.net/lists/arm-kernel/msg502238.html
> 
>  arch/arm64/kernel/process.c |   24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -350,15 +350,27 @@ unsigned long arch_align_stack(unsigned
>  	return sp & ~0xf;
>  }
>  
> -static unsigned long randomize_base(unsigned long base)
> +unsigned long arch_randomize_brk(struct mm_struct *mm)
>  {
> +	unsigned long base = mm->brk;
>  	unsigned long range_end = base + (STACK_RND_MASK << PAGE_SHIFT) + 1;
> -	return randomize_range(base, range_end, 0) ? : base;
> -}
> +	unsigned long max_stack, range_limit;
>  
> -unsigned long arch_randomize_brk(struct mm_struct *mm)
> -{
> -	return randomize_base(mm->brk);
> +	/*
> +	 * Determine how much room we need to leave available for the stack.
> +	 * We limit this to a reasonable value, because extremely large or
> +	 * unlimited stacks are always going to bump up against brk at some
> +	 * point and we don't want to fail to randomise brk in those cases.
> +	 */
> +	max_stack = rlimit(RLIMIT_STACK);
> +	if (max_stack > SZ_128M)
> +		max_stack = SZ_128M;
> +
> +	range_limit = mm->start_stack - max_stack - 1;
> +	if (range_end > range_limit)
> +		range_end = range_limit;
> +
> +	return randomize_range(base, range_end, 0) ? : base;
>  }
>  
>  unsigned long randomize_et_dyn(unsigned long base)
> 
> 

Powered by blists - more mailing lists