| Message ID | e2209d0f1f3c1b581592bd6c32243402ccfe3dde.1637570556.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert powerpc to default topdown mmap layout | expand |
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) > { >
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
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) {
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 <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(-)