| Message ID | 1461853042.2848.24.camel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 2016-05-02 at 12:34 -0700, Kees Cook wrote: > On Thu, Apr 28, 2016 at 7:17 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: [...] > > arch/arm64/kernel/process.c | 24 ++++++++++++++++++------ > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 07c4c53..7e0f404 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -434,13 +434,25 @@ unsigned long arch_align_stack(unsigned long sp) > > 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; > > This looks wrong. Shouldn't it be (STACK_RND_MASK + 1) << PAGE_SHIFT ? That value is the same as before my changes and it matches the gap left for stack randomisation in arch/arm64/mm/mmap.c > > STACK_RND_MASK is 0x7ff (32-bit) or 0x3ffff (64-bit): > > #define STACK_RND_MASK (test_thread_flag(TIF_32BIT) ? \ > 0x7ff >> (PAGE_SHIFT - 12) : \ > 0x3ffff >> (PAGE_SHIFT - 12)) > > (4K paged PAGE_SHIFT is 12) > > So the correct offset max would be 0x800000 (32-bit) and 0x40000000 > (64-bit), instead of > 0x7ff0001 and 0x3ffff0001. It seems to me there isn't a 'correct' and 'incorrect' range to use here and that randomising brk is not directly related to stack randomisation, they just have similar requirements and constraints. Anyway, for stack randomisation, in fs/binfmt_elf.c, randomize_stack_top() has random_variable = get_random_long(); random_variable &= STACK_RND_MASK; random_variable <<= PAGE_SHIFT; so stack top can be randomised by adding a number from zero to (STACK_RND_MASK << PAGE_SHIFT) inclusive. As the end value passed to randomize_range() is exclusive, then adding one to the last permissible value seems the like the right, I.e. arm64's usage of (STACK_RND_MASK << PAGE_SHIFT) + 1 for brk is 'correct' in that it's consistent with what happens to the stack. Though the different functions align values to pages at different stages, so possibly neither that nor (STACK_RND_MASK + 1) << PAGE_SHIFT when used for brk, would be the same as the stack code. > Even with that correction, this looks wrong for 32-bit, which uses > 0x2000000 natively: > > unsigned long arch_randomize_brk(struct mm_struct *mm) > { > unsigned long range_end = mm->brk + 0x02000000; > return randomize_range(mm->brk, range_end, 0) ? : mm->brk; > } > > Seems like arm64 compat is using 4 times less entropy than native arm? > (Note that STACK_RND_MASK is correct for arm64 compat: this matches > the default in fs/binfmt_elf.c that arm uses. It just seems like the > brk randomization is accidentally too small on arm64 compat since arm > uses a fixed value unrelated to stack randomization.) > > 0x02000000 arm native > 0x00800000 arm64 compat <- bug? > 0x40000000 arm64 Well, it's a difference for which there probably isn't a good reason, don't know if people would call it a bug. As changing the range of values used for randomisation seems like a separate issue I won't include any changes for that in my patch for getting brk to avoid the stack. -- Tixy _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 07c4c53..7e0f404 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -434,13 +434,25 @@ unsigned long arch_align_stack(unsigned long sp) 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 do 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; }