| Message ID | 20190526134746.9315-1-alex@ghiti.fr (mailing list archive) |
|---|---|
| Headers | show |
| Series | Provide generic top-down mmap layout functions | expand |
On Sun, May 26, 2019 at 09:47:44AM -0400, Alexandre Ghiti wrote: > Mips uses TASK_IS_32BIT_ADDR to determine if a task is 32bit, but > this define is mips specific and other arches do not have it: instead, > use !IS_ENABLED(CONFIG_64BIT) || is_compat_task() condition. > > Signed-off-by: Alexandre Ghiti <alex@ghiti.fr> Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > arch/mips/mm/mmap.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c > index c052565b76fb..900670ea8531 100644 > --- a/arch/mips/mm/mmap.c > +++ b/arch/mips/mm/mmap.c > @@ -17,6 +17,7 @@ > #include <linux/sched/signal.h> > #include <linux/sched/mm.h> > #include <linux/sizes.h> > +#include <linux/compat.h> > > unsigned long shm_align_mask = PAGE_SIZE - 1; /* Sane caches */ > EXPORT_SYMBOL(shm_align_mask); > @@ -191,7 +192,7 @@ static inline unsigned long brk_rnd(void) > > rnd = rnd << PAGE_SHIFT; > /* 32MB for 32bit, 1GB for 64bit */ > - if (TASK_IS_32BIT_ADDR) > + if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task()) > rnd = rnd & SZ_32M; > else > rnd = rnd & SZ_1G; > -- > 2.20.1 >
On Sun, May 26, 2019 at 09:47:32AM -0400, Alexandre Ghiti wrote: > This series introduces generic functions to make top-down mmap layout > easily accessible to architectures, in particular riscv which was > the initial goal of this series. > The generic implementation was taken from arm64 and used successively > by arm, mips and finally riscv. As I've mentioned before, I think this is really great. Making this common has long been on my TODO list. Thank you for the work! (I've sent separate review emails for individual patches where my ack wasn't already present...) > - There is no common API to determine if a process is 32b, so I came up with > !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14]. Do we need a common helper for this idiom? (Note that I don't think it's worth blocking the series for this.) -Kees
On 5/29/19 4:16 PM, Kees Cook wrote: > On Sun, May 26, 2019 at 09:47:32AM -0400, Alexandre Ghiti wrote: >> This series introduces generic functions to make top-down mmap layout >> easily accessible to architectures, in particular riscv which was >> the initial goal of this series. >> The generic implementation was taken from arm64 and used successively >> by arm, mips and finally riscv. > As I've mentioned before, I think this is really great. Making this > common has long been on my TODO list. Thank you for the work! (I've sent > separate review emails for individual patches where my ack wasn't > already present...) Thanks :) >> - There is no common API to determine if a process is 32b, so I came up with >> !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14]. > Do we need a common helper for this idiom? (Note that I don't think it's > worth blocking the series for this.) Each architecture has its own way of finding that out, it might be interesting if there are other places in generic code to propose something in that sense. I will search for such places if they exist and come back with something. Thanks Kees for your time, Alex > > -Kees >
On 5/26/19 9:47 AM, Alexandre Ghiti wrote: > This series introduces generic functions to make top-down mmap layout > easily accessible to architectures, in particular riscv which was > the initial goal of this series. > The generic implementation was taken from arm64 and used successively > by arm, mips and finally riscv. > > Note that in addition the series fixes 2 issues: > - stack randomization was taken into account even if not necessary. > - [1] fixed an issue with mmap base which did not take into account > randomization but did not report it to arm and mips, so by moving > arm64 into a generic library, this problem is now fixed for both > architectures. > > This work is an effort to factorize architecture functions to avoid > code duplication and oversights as in [1]. > > [1]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1429066.html > > Changes in v4: > - Make ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select ARCH_HAS_ELF_RANDOMIZE > by default as suggested by Kees, > - ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT depends on MMU and defines the > functions needed by ARCH_HAS_ELF_RANDOMIZE => architectures that use > the generic mmap topdown functions cannot have ARCH_HAS_ELF_RANDOMIZE > selected without MMU, but I think it's ok since randomization without > MMU does not add much security anyway. > - There is no common API to determine if a process is 32b, so I came up with > !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14]. > - Mention in the change log that x86 already takes care of not offseting mmap > base address if the task does not want randomization. > - Re-introduce a comment that should not have been removed. > - Add Reviewed/Acked-By from Paul, Christoph and Kees, thank you for that. > - I tried to minimize the changes from the commits in v3 in order to make > easier the review of the v4, the commits changed or added are: > - [PATCH v4 5/14] > - [PATCH v4 8/14] > - [PATCH v4 11/14] > - [PATCH v4 12/14] > - [PATCH v4 13/14] Hi Paul, Compared to the previous version you already acked, patches 11, 12 and 13 would need your feedback, do you have time to take a look at them ? Hope I don't bother you, Thanks, Alex > > Changes in v3: > - Split into small patches to ease review as suggested by Christoph > Hellwig and Kees Cook > - Move help text of new config as a comment, as suggested by Christoph > - Make new config depend on MMU, as suggested by Christoph > > Changes in v2 as suggested by Christoph Hellwig: > - Preparatory patch that moves randomize_stack_top > - Fix duplicate config in riscv > - Align #if defined on next line => this gives rise to a checkpatch > warning. I found this pattern all around the tree, in the same proportion > as the previous pattern which was less pretty: > git grep -C 1 -n -P "^#if defined.+\|\|.*\\\\$" > > Alexandre Ghiti (14): > mm, fs: Move randomize_stack_top from fs to mm > arm64: Make use of is_compat_task instead of hardcoding this test > arm64: Consider stack randomization for mmap base only when necessary > arm64, mm: Move generic mmap layout functions to mm > arm64, mm: Make randomization selected by generic topdown mmap layout > arm: Properly account for stack randomization and stack guard gap > arm: Use STACK_TOP when computing mmap base address > arm: Use generic mmap top-down layout and brk randomization > mips: Properly account for stack randomization and stack guard gap > mips: Use STACK_TOP when computing mmap base address > mips: Adjust brk randomization offset to fit generic version > mips: Replace arch specific way to determine 32bit task with generic > version > mips: Use generic mmap top-down layout and brk randomization > riscv: Make mmap allocation top-down by default > > arch/Kconfig | 11 +++ > arch/arm/Kconfig | 2 +- > arch/arm/include/asm/processor.h | 2 - > arch/arm/kernel/process.c | 5 -- > arch/arm/mm/mmap.c | 52 -------------- > arch/arm64/Kconfig | 2 +- > arch/arm64/include/asm/processor.h | 2 - > arch/arm64/kernel/process.c | 8 --- > arch/arm64/mm/mmap.c | 72 ------------------- > arch/mips/Kconfig | 2 +- > arch/mips/include/asm/processor.h | 5 -- > arch/mips/mm/mmap.c | 84 ---------------------- > arch/riscv/Kconfig | 11 +++ > fs/binfmt_elf.c | 20 ------ > include/linux/mm.h | 2 + > kernel/sysctl.c | 6 +- > mm/util.c | 107 ++++++++++++++++++++++++++++- > 17 files changed, 137 insertions(+), 256 deletions(-) >
On 6/13/19 1:29 AM, Alex Ghiti wrote: > On 5/26/19 9:47 AM, Alexandre Ghiti wrote: >> This series introduces generic functions to make top-down mmap layout >> easily accessible to architectures, in particular riscv which was >> the initial goal of this series. >> The generic implementation was taken from arm64 and used successively >> by arm, mips and finally riscv. >> >> Note that in addition the series fixes 2 issues: >> - stack randomization was taken into account even if not necessary. >> - [1] fixed an issue with mmap base which did not take into account >> randomization but did not report it to arm and mips, so by moving >> arm64 into a generic library, this problem is now fixed for both >> architectures. >> >> This work is an effort to factorize architecture functions to avoid >> code duplication and oversights as in [1]. >> >> [1]: >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1429066.html >> >> Changes in v4: >> - Make ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select >> ARCH_HAS_ELF_RANDOMIZE >> by default as suggested by Kees, >> - ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT depends on MMU and defines >> the >> functions needed by ARCH_HAS_ELF_RANDOMIZE => architectures that >> use >> the generic mmap topdown functions cannot have >> ARCH_HAS_ELF_RANDOMIZE >> selected without MMU, but I think it's ok since randomization >> without >> MMU does not add much security anyway. >> - There is no common API to determine if a process is 32b, so I >> came up with >> !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14]. >> - Mention in the change log that x86 already takes care of not >> offseting mmap >> base address if the task does not want randomization. >> - Re-introduce a comment that should not have been removed. >> - Add Reviewed/Acked-By from Paul, Christoph and Kees, thank you >> for that. >> - I tried to minimize the changes from the commits in v3 in order >> to make >> easier the review of the v4, the commits changed or added are: >> - [PATCH v4 5/14] >> - [PATCH v4 8/14] >> - [PATCH v4 11/14] >> - [PATCH v4 12/14] >> - [PATCH v4 13/14] > > Hi Paul, > > Compared to the previous version you already acked, patches 11, 12 and 13 > would need your feedback, do you have time to take a look at them ? > > Hope I don't bother you, > > Thanks, > > Alex > Hi Paul, Would you have time to give your feedback on patches 11, 12 and 13 ? Thanks, Alex > >> >> Changes in v3: >> - Split into small patches to ease review as suggested by Christoph >> Hellwig and Kees Cook >> - Move help text of new config as a comment, as suggested by >> Christoph >> - Make new config depend on MMU, as suggested by Christoph >> >> Changes in v2 as suggested by Christoph Hellwig: >> - Preparatory patch that moves randomize_stack_top >> - Fix duplicate config in riscv >> - Align #if defined on next line => this gives rise to a checkpatch >> warning. I found this pattern all around the tree, in the same >> proportion >> as the previous pattern which was less pretty: >> git grep -C 1 -n -P "^#if defined.+\|\|.*\\\\$" >> >> Alexandre Ghiti (14): >> mm, fs: Move randomize_stack_top from fs to mm >> arm64: Make use of is_compat_task instead of hardcoding this test >> arm64: Consider stack randomization for mmap base only when necessary >> arm64, mm: Move generic mmap layout functions to mm >> arm64, mm: Make randomization selected by generic topdown mmap layout >> arm: Properly account for stack randomization and stack guard gap >> arm: Use STACK_TOP when computing mmap base address >> arm: Use generic mmap top-down layout and brk randomization >> mips: Properly account for stack randomization and stack guard gap >> mips: Use STACK_TOP when computing mmap base address >> mips: Adjust brk randomization offset to fit generic version >> mips: Replace arch specific way to determine 32bit task with generic >> version >> mips: Use generic mmap top-down layout and brk randomization >> riscv: Make mmap allocation top-down by default >> >> arch/Kconfig | 11 +++ >> arch/arm/Kconfig | 2 +- >> arch/arm/include/asm/processor.h | 2 - >> arch/arm/kernel/process.c | 5 -- >> arch/arm/mm/mmap.c | 52 -------------- >> arch/arm64/Kconfig | 2 +- >> arch/arm64/include/asm/processor.h | 2 - >> arch/arm64/kernel/process.c | 8 --- >> arch/arm64/mm/mmap.c | 72 ------------------- >> arch/mips/Kconfig | 2 +- >> arch/mips/include/asm/processor.h | 5 -- >> arch/mips/mm/mmap.c | 84 ---------------------- >> arch/riscv/Kconfig | 11 +++ >> fs/binfmt_elf.c | 20 ------ >> include/linux/mm.h | 2 + >> kernel/sysctl.c | 6 +- >> mm/util.c | 107 ++++++++++++++++++++++++++++- >> 17 files changed, 137 insertions(+), 256 deletions(-) >> > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
This series introduces generic functions to make top-down mmap layout easily accessible to architectures, in particular riscv which was the initial goal of this series. The generic implementation was taken from arm64 and used successively by arm, mips and finally riscv. Note that in addition the series fixes 2 issues: - stack randomization was taken into account even if not necessary. - [1] fixed an issue with mmap base which did not take into account randomization but did not report it to arm and mips, so by moving arm64 into a generic library, this problem is now fixed for both architectures. This work is an effort to factorize architecture functions to avoid code duplication and oversights as in [1]. [1]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1429066.html Changes in v4: - Make ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT select ARCH_HAS_ELF_RANDOMIZE by default as suggested by Kees, - ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT depends on MMU and defines the functions needed by ARCH_HAS_ELF_RANDOMIZE => architectures that use the generic mmap topdown functions cannot have ARCH_HAS_ELF_RANDOMIZE selected without MMU, but I think it's ok since randomization without MMU does not add much security anyway. - There is no common API to determine if a process is 32b, so I came up with !IS_ENABLED(CONFIG_64BIT) || is_compat_task() in [PATCH v4 12/14]. - Mention in the change log that x86 already takes care of not offseting mmap base address if the task does not want randomization. - Re-introduce a comment that should not have been removed. - Add Reviewed/Acked-By from Paul, Christoph and Kees, thank you for that. - I tried to minimize the changes from the commits in v3 in order to make easier the review of the v4, the commits changed or added are: - [PATCH v4 5/14] - [PATCH v4 8/14] - [PATCH v4 11/14] - [PATCH v4 12/14] - [PATCH v4 13/14] Changes in v3: - Split into small patches to ease review as suggested by Christoph Hellwig and Kees Cook - Move help text of new config as a comment, as suggested by Christoph - Make new config depend on MMU, as suggested by Christoph Changes in v2 as suggested by Christoph Hellwig: - Preparatory patch that moves randomize_stack_top - Fix duplicate config in riscv - Align #if defined on next line => this gives rise to a checkpatch warning. I found this pattern all around the tree, in the same proportion as the previous pattern which was less pretty: git grep -C 1 -n -P "^#if defined.+\|\|.*\\\\$" Alexandre Ghiti (14): mm, fs: Move randomize_stack_top from fs to mm arm64: Make use of is_compat_task instead of hardcoding this test arm64: Consider stack randomization for mmap base only when necessary arm64, mm: Move generic mmap layout functions to mm arm64, mm: Make randomization selected by generic topdown mmap layout arm: Properly account for stack randomization and stack guard gap arm: Use STACK_TOP when computing mmap base address arm: Use generic mmap top-down layout and brk randomization mips: Properly account for stack randomization and stack guard gap mips: Use STACK_TOP when computing mmap base address mips: Adjust brk randomization offset to fit generic version mips: Replace arch specific way to determine 32bit task with generic version mips: Use generic mmap top-down layout and brk randomization riscv: Make mmap allocation top-down by default arch/Kconfig | 11 +++ arch/arm/Kconfig | 2 +- arch/arm/include/asm/processor.h | 2 - arch/arm/kernel/process.c | 5 -- arch/arm/mm/mmap.c | 52 -------------- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/processor.h | 2 - arch/arm64/kernel/process.c | 8 --- arch/arm64/mm/mmap.c | 72 ------------------- arch/mips/Kconfig | 2 +- arch/mips/include/asm/processor.h | 5 -- arch/mips/mm/mmap.c | 84 ---------------------- arch/riscv/Kconfig | 11 +++ fs/binfmt_elf.c | 20 ------ include/linux/mm.h | 2 + kernel/sysctl.c | 6 +- mm/util.c | 107 ++++++++++++++++++++++++++++- 17 files changed, 137 insertions(+), 256 deletions(-)