On Thu, Feb 26, 2015 at 2:38 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 24 Feb 2015 08:39:06 +0100 Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>>
>> * Hector Marco Gisbert <hecmargi@xxxxxx> wrote:
>>
>> > +unsigned long randomize_et_dyn(unsigned long base)
>> > +{
>> > + unsigned long ret;
>> > + if ((current->personality & ADDR_NO_RANDOMIZE) ||
>> > + !(current->flags & PF_RANDOMIZE))
>> > + return base;
>> > + ret = base + mmap_rnd();
>> > + return (ret > base) ? ret : base;
>> > +}
>>
>> > +unsigned long randomize_et_dyn(unsigned long base)
>> > +{
>> > + unsigned long ret;
>> > + if ((current->personality & ADDR_NO_RANDOMIZE) ||
>> > + !(current->flags & PF_RANDOMIZE))
>> > + return base;
>> > + ret = base + mmap_rnd();
>> > + return (ret > base) ? ret : base;
>> > +}
>>
>> > +unsigned long randomize_et_dyn(unsigned long base)
>> > +{
>> > + unsigned long ret;
>> > + if ((current->personality & ADDR_NO_RANDOMIZE) ||
>> > + !(current->flags & PF_RANDOMIZE))
>> > + return base;
>> > + ret = base + brk_rnd();
>> > + return (ret > base) ? ret : base;
>> > +}
>>
>> > +unsigned long randomize_et_dyn(unsigned long base)
>> > +{
>> > + unsigned long ret;
>> > + if ((current->personality & ADDR_NO_RANDOMIZE) ||
>> > + !(current->flags & PF_RANDOMIZE))
>> > + return base;
>> > + ret = base + mmap_rnd();
>> > + return (ret > base) ? ret : base;
>> > +}
>>
>> > +unsigned long randomize_et_dyn(unsigned long base)
>> > +{
>> > + unsigned long ret;
>> > + if ((current->personality & ADDR_NO_RANDOMIZE) ||
>> > + !(current->flags & PF_RANDOMIZE))
>> > + return base;
>> > + ret = base + mmap_rnd();
>> > + return (ret > base) ? ret : base;
>> > +}
>>
>> That pointless repetition should be avoided.
>
> That's surprisingly hard!
>
> After renaming mips brk_rnd() to mmap_rnd() I had a shot. I'm not very
> confident in the result. Does that __weak trick even work?
In theory, it shouldn't be needed since only randomize_et_dyn will
call mmap_rnd, and only architectures that use randomize_et_dyn will
call it ... and will define mmap_rnd.
>
> Someone tell me how important Hector's patch is?
I consider it a reasonable improvement to userspace ASLR. I look at it
more as a new feature than a bug fix, but it could be argued as a bug
fix too.
>
>
> From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Subject: fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
>
> Consolidate randomize_et_dyn() implementations into fs/binfmt_elf.c.
>
> There doesn't seem to be a compile-time way of making randomize_et_dyn()
> go away on architectures which don't need it, so mark it __weak to cause
> it to be discarded at link time.
>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Hector Marco Gisbert <hecmargi@xxxxxx>
> Cc: Hector Marco-Gisbert <hecmargi@xxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Ismael Ripoll <iripoll@xxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: Russell King <rmk@xxxxxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
Thanks for fixing it up!
-Kees
> ---
>
> arch/arm/include/asm/elf.h | 3 +--
> arch/arm/mm/mmap.c | 13 ++-----------
> arch/arm64/Kconfig | 2 +-
> arch/arm64/include/asm/elf.h | 3 +--
> arch/arm64/mm/mmap.c | 14 ++------------
> arch/mips/include/asm/elf.h | 1 -
> arch/mips/mm/mmap.c | 13 ++-----------
> arch/powerpc/include/asm/elf.h | 2 --
> arch/powerpc/mm/mmap.c | 13 ++-----------
> arch/x86/include/asm/elf.h | 2 --
> arch/x86/mm/mmap.c | 12 ++----------
> fs/binfmt_elf.c | 21 +++++++++++++++++++++
> include/linux/elf-randomization.h | 7 +++++++
> 13 files changed, 41 insertions(+), 65 deletions(-)
>
> diff -puN arch/arm/Kconfig~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/arm/Kconfig
> diff -puN arch/arm/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/arm/include/asm/elf.h
> --- a/arch/arm/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/arm/include/asm/elf.h
> @@ -2,7 +2,7 @@
> #define __ASMARM_ELF_H
>
> #include <asm/hwcap.h>
> -
> +#include <linux/elf-randomization.h>
> /*
> * ELF register definitions..
> */
> @@ -115,7 +115,6 @@ int dump_task_regs(struct task_struct *t
> the loader. We need to make sure that it is out of the way of the program
> that it will "exec", and that there is sufficient room for the brk. */
>
> -extern unsigned long randomize_et_dyn(unsigned long base);
> #define ELF_ET_DYN_BASE (randomize_et_dyn(2 * TASK_SIZE / 3))
>
> /* When the program starts, a1 contains a pointer to a function to be
> diff -puN arch/arm/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/arm/mm/mmap.c
> --- a/arch/arm/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/arm/mm/mmap.c
> @@ -7,6 +7,7 @@
> #include <linux/shm.h>
> #include <linux/sched.h>
> #include <linux/io.h>
> +#include <linux/elf-randomization.h>
> #include <linux/personality.h>
> #include <linux/random.h>
> #include <asm/cachetype.h>
> @@ -30,7 +31,7 @@ static int mmap_is_legacy(void)
> return sysctl_legacy_va_layout;
> }
>
> -static unsigned long mmap_rnd(void)
> +unsigned long mmap_rnd(void)
> {
> unsigned long rnd = 0;
>
> @@ -241,13 +242,3 @@ int devmem_is_allowed(unsigned long pfn)
> }
>
> #endif
> -
> -unsigned long randomize_et_dyn(unsigned long base)
> -{
> - unsigned long ret;
> - if ((current->personality & ADDR_NO_RANDOMIZE) ||
> - !(current->flags & PF_RANDOMIZE))
> - return base;
> - ret = base + mmap_rnd();
> - return (ret > base) ? ret : base;
> -}
> diff -puN arch/arm64/Kconfig~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/arm64/Kconfig
> --- a/arch/arm64/Kconfig~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/arm64/Kconfig
> @@ -1,4 +1,4 @@
> -config ARM64
> +qconfig ARM64
> def_bool y
> select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
> select ARCH_HAS_GCOV_PROFILE_ALL
> diff -puN arch/arm64/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/arm64/include/asm/elf.h
> --- a/arch/arm64/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/arm64/include/asm/elf.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2012 ARM Ltd.
> + * Copyright (C) 20q12 ARM Ltd.
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -125,7 +125,6 @@ typedef struct user_fpsimd_state elf_fpr
> * the loader. We need to make sure that it is out of the way of the program
> * that it will "exec", and that there is sufficient room for the brk.
> */
> -extern unsigned long randomize_et_dyn(unsigned long base);
> #define ELF_ET_DYN_BASE (randomize_et_dyn(2 * TASK_SIZE_64 / 3))
>
> /*
> diff -puN arch/arm64/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/arm64/mm/mmap.c
> --- a/arch/arm64/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/arm64/mm/mmap.c
> @@ -17,6 +17,7 @@
> */
>
> #include <linux/elf.h>
> +#include <linux/elf-randomization.h>
> #include <linux/fs.h>
> #include <linux/mm.h>
> #include <linux/mman.h>
> @@ -47,7 +48,7 @@ static int mmap_is_legacy(void)
> return sysctl_legacy_va_layout;
> }
>
> -static unsigned long mmap_rnd(void)
> +unsigned long mmap_rnd(void)
> {
> unsigned long rnd = 0;
>
> @@ -89,17 +90,6 @@ void arch_pick_mmap_layout(struct mm_str
> }
> EXPORT_SYMBOL_GPL(arch_pick_mmap_layout);
>
> -unsigned long randomize_et_dyn(unsigned long base)
> -{
> - unsigned long ret;
> - if ((current->personality & ADDR_NO_RANDOMIZE) ||
> - !(current->flags & PF_RANDOMIZE))
> - return base;
> - ret = base + mmap_rnd();
> - return (ret > base) ? ret : base;
> -}
> -
> -
> /*
> * You really shouldn't be using read() or write() on /dev/mem. This might go
> * away in the future.
> diff -puN arch/mips/Kconfig~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/mips/Kconfig
> diff -puN arch/mips/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/mips/include/asm/elf.h
> --- a/arch/mips/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/mips/include/asm/elf.h
> @@ -402,7 +402,6 @@ extern const char *__elf_platform;
> that it will "exec", and that there is sufficient room for the brk. */
>
> #ifndef ELF_ET_DYN_BASE
> -extern unsigned long randomize_et_dyn(unsigned long base);
> #define ELF_ET_DYN_BASE (randomize_et_dyn(TASK_SIZE / 3 * 2))
> #endif
>
> diff -puN arch/mips/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/mips/mm/mmap.c
> --- a/arch/mips/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/mips/mm/mmap.c
> @@ -9,6 +9,7 @@
> #include <linux/compiler.h>
> #include <linux/errno.h>
> #include <linux/mm.h>
> +#include <linux/elf-randomization.h>
> #include <linux/mman.h>
> #include <linux/module.h>
> #include <linux/personality.h>
> @@ -164,7 +165,7 @@ void arch_pick_mmap_layout(struct mm_str
> }
> }
>
> -static inline unsigned long mmap_rnd(void)
> +unsigned long mmap_rnd(void)
> {
> unsigned long rnd = get_random_int();
>
> @@ -196,13 +197,3 @@ int __virt_addr_valid(const volatile voi
> return pfn_valid(PFN_DOWN(virt_to_phys(kaddr)));
> }
> EXPORT_SYMBOL_GPL(__virt_addr_valid);
> -
> -unsigned long randomize_et_dyn(unsigned long base)
> -{
> - unsigned long ret;
> - if ((current->personality & ADDR_NO_RANDOMIZE) ||
> - !(current->flags & PF_RANDOMIZE))
> - return base;
> - ret = base + brk_rnd();
> - return (ret > base) ? ret : base;
> -}
> diff -puN arch/powerpc/Kconfig~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/powerpc/Kconfig
> diff -puN arch/powerpc/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/powerpc/include/asm/elf.h
> --- a/arch/powerpc/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/powerpc/include/asm/elf.h
> @@ -27,8 +27,6 @@
> use of this is to invoke "./ld.so someprog" to test out a new version of
> the loader. We need to make sure that it is out of the way of the program
> that it will "exec", and that there is sufficient room for the brk. */
> -
> -extern unsigned long randomize_et_dyn(unsigned long base);
> #define ELF_ET_DYN_BASE (randomize_et_dyn(0x20000000))
>
> #define ELF_CORE_EFLAGS (is_elf2_task() ? 2 : 0)
> diff -puN arch/powerpc/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/powerpc/mm/mmap.c
> --- a/arch/powerpc/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/powerpc/mm/mmap.c
> @@ -24,6 +24,7 @@
>
> #include <linux/personality.h>
> #include <linux/mm.h>
> +#include <linux/elf-randomization.h>
> #include <linux/random.h>
> #include <linux/sched.h>
>
> @@ -53,7 +54,7 @@ static inline int mmap_is_legacy(void)
> return sysctl_legacy_va_layout;
> }
>
> -static unsigned long mmap_rnd(void)
> +unsigned long mmap_rnd(void)
> {
> unsigned long rnd = 0;
>
> @@ -97,13 +98,3 @@ void arch_pick_mmap_layout(struct mm_str
> mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> }
> }
> -
> -unsigned long randomize_et_dyn(unsigned long base)
> -{
> - unsigned long ret;
> - if ((current->personality & ADDR_NO_RANDOMIZE) ||
> - !(current->flags & PF_RANDOMIZE))
> - return base;
> - ret = base + mmap_rnd();
> - return (ret > base) ? ret : base;
> -}
> diff -puN arch/x86/Kconfig~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/x86/Kconfig
> diff -puN arch/x86/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/x86/include/asm/elf.h
> --- a/arch/x86/include/asm/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/x86/include/asm/elf.h
> @@ -248,8 +248,6 @@ extern int force_personality32;
> use of this is to invoke "./ld.so someprog" to test out a new version of
> the loader. We need to make sure that it is out of the way of the program
> that it will "exec", and that there is sufficient room for the brk. */
> -
> -extern unsigned long randomize_et_dyn(unsigned long base);
> #define ELF_ET_DYN_BASE (randomize_et_dyn(TASK_SIZE / 3 * 2))
>
> /* This yields a mask that user programs can use to figure out what
> diff -puN arch/x86/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix arch/x86/mm/mmap.c
> --- a/arch/x86/mm/mmap.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/arch/x86/mm/mmap.c
> @@ -29,6 +29,7 @@
> #include <linux/random.h>
> #include <linux/limits.h>
> #include <linux/sched.h>
> +#include <linux/elf-randomization.h>
> #include <asm/elf.h>
>
> struct va_alignment __read_mostly va_align = {
> @@ -65,7 +66,7 @@ static int mmap_is_legacy(void)
> return sysctl_legacy_va_layout;
> }
>
> -static unsigned long mmap_rnd(void)
> +unsigned long mmap_rnd(void)
> {
> unsigned long rnd = 0;
>
> @@ -122,12 +123,3 @@ void arch_pick_mmap_layout(struct mm_str
> mm->get_unmapped_area = arch_get_unmapped_area_topdown;
> }
> }
> -unsigned long randomize_et_dyn(unsigned long base)
> -{
> - unsigned long ret;
> - if ((current->personality & ADDR_NO_RANDOMIZE) ||
> - !(current->flags & PF_RANDOMIZE))
> - return base;
> - ret = base + mmap_rnd();
> - return (ret > base) ? ret : base;
> -}
> diff -puN fs/Kconfig.binfmt~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix fs/Kconfig.binfmt
> diff -puN fs/binfmt_elf.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix fs/binfmt_elf.c
> --- a/fs/binfmt_elf.c~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix
> +++ a/fs/binfmt_elf.c
> @@ -22,6 +22,7 @@
> #include <linux/slab.h>
> #include <linux/personality.h>
> #include <linux/elfcore.h>
> +#include <linux/elf-randomization.h>
> #include <linux/init.h>
> #include <linux/highuid.h>
> #include <linux/compiler.h>
> @@ -2300,6 +2301,26 @@ out:
>
> #endif /* CONFIG_ELF_CORE */
>
> +/* Not all architectures implement mmap_rnd() */
> +unsigned long __weak mmap_rnd(void)
> +{
> +}
> +
> +/*
> + * Not all architectures use randomize_et_dyn(), so use __weak to let the
> + * linker omit it from vmlinux
> + */
> +unsigned long __weak randomize_et_dyn(unsigned long base)
> +{
> + unsigned long ret;
> +
> + if ((current->personality & ADDR_NO_RANDOMIZE) ||
> + !(current->flags & PF_RANDOMIZE))
> + return base;
> + ret = base + mmap_rnd();
> + return max(ret, base);
> +}
> +
> static int __init init_elf_binfmt(void)
> {
> register_binfmt(&elf_format);
> diff -puN include/linux/elf.h~fix-offset2lib-issue-for-x86-arm-powerpc-and-mips-fix include/linux/elf.h
> diff -puN /dev/null include/linux/elf-randomization.h
> --- /dev/null
> +++ a/include/linux/elf-randomization.h
> @@ -0,0 +1,7 @@
> +#ifndef __ELF_RANDOMIZATION_H
> +#define __ELF_RANDOMIZATION_H
> +
> +unsigned long randomize_et_dyn(unsigned long base);
> +unsigned long mmap_rnd(void);
> +
> +#endif /* __ELF_RANDOMIZATION_H */
> _
>
--
Kees Cook
Chrome OS Security
