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.
Then only provide the default arch_randomize_brk() when the
architecture has not selected CONFIG_ARCH_HAS_ELF_RANDOMIZE.
Cc: Alexandre Ghiti <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
---
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)
{
--
2.33.1
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 <[email protected]>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> 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)
> {
>
Le 22/11/2021 à 12:22, Alex Ghiti a écrit :
> 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?
Yes I also found things a bit weird.
CONFIG_ARCH_HAS_RANDOMIZE_BRK could be an idea but how different would
it be from CONFIG_ARCH_HAS_ELF_RANDOMIZE ? In fact I find it weird that
CONFIG_ARCH_HAS_ELF_RANDOMIZE is selected by
CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and not by the arch itself.
On the other hand CONFIG_ARCH_HAS_ELF_RANDOMIZE also handles
arch_mmap_rnd() and here we are talking about arch_randomize_brk() only.
In the begining I was thinking about adding a
CONFIG_ARCH_WANT_DEFAULT_RANDOMIZE_BRK, but it was meaning adding it to
the few other arches selecting CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT.
So I think I will go for the __weak function option.
Thanks
Christophe
On 11/22/21 12:47, Christophe Leroy wrote:
>
>
> Le 22/11/2021 à 12:22, Alex Ghiti a écrit :
>> 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?
>
>
> Yes I also found things a bit weird.
>
> CONFIG_ARCH_HAS_RANDOMIZE_BRK could be an idea but how different would
> it be from CONFIG_ARCH_HAS_ELF_RANDOMIZE ? In fact I find it weird
> that CONFIG_ARCH_HAS_ELF_RANDOMIZE is selected by
> CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and not by the arch itself.
IIRC, this was a request from Kees Cook who wanted to enforce this
security measure.
>
> On the other hand CONFIG_ARCH_HAS_ELF_RANDOMIZE also handles
> arch_mmap_rnd() and here we are talking about arch_randomize_brk() only.
>
> In the begining I was thinking about adding a
> CONFIG_ARCH_WANT_DEFAULT_RANDOMIZE_BRK, but it was meaning adding it
> to the few other arches selecting
> CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT.
>
> So I think I will go for the __weak function option.
Ok, thanks.
Alex
>
> Thanks
> Christophe
Hi Christophe,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on hnaz-mm/master linus/master v5.16-rc2 next-20211118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/Convert-powerpc-to-default-topdown-mmap-layout/20211122-165115
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: arm-randconfig-r005-20211122 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e5949ff1a8e5cae8e9ac2ec3a39849bf2e73eb34
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/Convert-powerpc-to-default-topdown-mmap-layout/20211122-165115
git checkout e5949ff1a8e5cae8e9ac2ec3a39849bf2e73eb34
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: fs/binfmt_elf.o: in function `load_elf_binary':
>> binfmt_elf.c:(.text+0x16d8): undefined reference to `arch_randomize_brk'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]