Re: [PATCH 6/8] mm: Allow arch specific arch_randomize_brk() with CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT

Alex Ghiti
2021-11-22T11:23:00Z

Hi Christophe,

Le 22/11/2021 à 09:48, Christophe Leroy a écrit :
> Commit e7142bf5d231 ("arm64, mm: make randomization selected by
> generic topdown mmap layout") introduced a default version of
> arch_randomize_brk() provided when
> CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT is selected.
>
> powerpc could select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> but needs to provide its own arch_randomize_brk().
>
> In order to allow that, don't make
> CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select
> CONFIG_ARCH_HAS_ELF_RANDOMIZE. Instead, ensure that
> selecting CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and
> selecting CONFIG_ARCH_HAS_ELF_RANDOMIZE has the same effect.

This feels weird to me since if CONFIG_ARCH_HAS_ELF_RANDOMIZE is used
somewhere else at some point, it is not natural to add
CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT: can't we use a __weak
function or a new CONFIG_ARCH_HAS_RANDOMIZE_BRK?

Thanks,

Alex

>
> Then only provide the default arch_randomize_brk() when the
> architecture has not selected CONFIG_ARCH_HAS_ELF_RANDOMIZE.
>
> Cc: Alexandre Ghiti <alex@ghiti.fr>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/Kconfig | 1 -
> fs/binfmt_elf.c | 3 ++-
> include/linux/elf-randomize.h | 3 ++-
> mm/util.c | 2 ++
> 4 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 26b8ed11639d..ef3ce947b7a1 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1000,7 +1000,6 @@ config HAVE_ARCH_COMPAT_MMAP_BASES
> config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> bool
> depends on MMU
> - select ARCH_HAS_ELF_RANDOMIZE
>
> config HAVE_STACK_VALIDATION
> bool
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index f8c7f26f1fbb..28968a189a91 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1287,7 +1287,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
> * (since it grows up, and may collide early with the stack
> * growing down), and into the unused ELF_ET_DYN_BASE region.
> */
> - if (IS_ENABLED(CONFIG_ARCH_HAS_ELF_RANDOMIZE) &&
> + if ((IS_ENABLED(CONFIG_ARCH_HAS_ELF_RANDOMIZE) ||
> + IS_ENABLED(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)) &&
> elf_ex->e_type == ET_DYN && !interpreter) {
> mm->brk = mm->start_brk = ELF_ET_DYN_BASE;
> }
> diff --git a/include/linux/elf-randomize.h b/include/linux/elf-randomize.h
> index da0dbb7b6be3..1e471ca7caaf 100644
> --- a/include/linux/elf-randomize.h
> +++ b/include/linux/elf-randomize.h
> @@ -4,7 +4,8 @@
>
> struct mm_struct;
>
> -#ifndef CONFIG_ARCH_HAS_ELF_RANDOMIZE
> +#if !defined(CONFIG_ARCH_HAS_ELF_RANDOMIZE) && \
> + !defined(CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT)
> static inline unsigned long arch_mmap_rnd(void) { return 0; }
> # if defined(arch_randomize_brk) && defined(CONFIG_COMPAT_BRK)
> # define compat_brk_randomized
> diff --git a/mm/util.c b/mm/util.c
> index e58151a61255..edb9e94cceb5 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -344,6 +344,7 @@ unsigned long randomize_stack_top(unsigned long stack_top)
> }
>
> #ifdef CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> +#ifndef CONFIG_ARCH_HAS_ELF_RANDOMIZE
> unsigned long arch_randomize_brk(struct mm_struct *mm)
> {
> /* Is the current task 32bit ? */
> @@ -352,6 +353,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>
> return randomize_page(mm->brk, SZ_1G);
> }
> +#endif
>
> unsigned long arch_mmap_rnd(void)
> {
>