| Message ID | 1500015083-23511-3-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 029d9252b116fa52a95150819e62af1f6e420fe5 |
| Headers | show |
Hi Michael,
[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20170714]
[cannot apply to v4.12]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Michael-Ellerman/powerpc-mm-radix-Refactor-radix__mark_rodata_ro/20170715-043340
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allnoconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/mm/mem.c: In function 'free_initmem':
>> arch/powerpc/mm/mem.c:413:2: error: implicit declaration of function 'mark_initmem_nx' [-Werror=implicit-function-declaration]
mark_initmem_nx();
^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/mark_initmem_nx +413 arch/powerpc/mm/mem.c
409
410 void free_initmem(void)
411 {
412 ppc_md.progress = ppc_printk_progress;
> 413 mark_initmem_nx();
414 free_initmem_default(POISON_FREE_INITMEM);
415 }
416
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Le 14/07/2017 à 08:51, Michael Ellerman a écrit : > Currently even with STRICT_KERNEL_RWX we leave the __init text marked > executable after init, which is bad. > > Add a hook to mark it NX (no-execute) before we free it, and implement > it for radix and hash. > > Note that we use __init_end as the end address, not _einittext, > because overlaps_kernel_text() uses __init_end, because there are > additional executable sections other than .init.text between > __init_begin and __init_end. > > Tested on radix and hash with: > > 0:mon> p $__init_begin > *** 400 exception occurred > > Fixes: 1e0fc9d1eb2b ("powerpc/Kconfig: Enable STRICT_KERNEL_RWX for some configs") > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/include/asm/book3s/64/hash.h | 1 + > arch/powerpc/include/asm/book3s/64/pgtable.h | 7 +++++++ > arch/powerpc/include/asm/book3s/64/radix.h | 1 + > arch/powerpc/mm/mem.c | 1 + > arch/powerpc/mm/pgtable-hash64.c | 12 ++++++++++++ > arch/powerpc/mm/pgtable-radix.c | 8 ++++++++ > arch/powerpc/mm/pgtable_64.c | 8 ++++++++ > 7 files changed, 38 insertions(+) > > diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h > index 0ce513f2926f..36fc7bfe9e11 100644 > --- a/arch/powerpc/include/asm/book3s/64/hash.h > +++ b/arch/powerpc/include/asm/book3s/64/hash.h > @@ -91,6 +91,7 @@ static inline int hash__pgd_bad(pgd_t pgd) > } > #ifdef CONFIG_STRICT_KERNEL_RWX > extern void hash__mark_rodata_ro(void); > +extern void hash__mark_initmem_nx(void); > #endif > > extern void hpte_need_flush(struct mm_struct *mm, unsigned long addr, > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h > index c0737c86a362..3d562b210c65 100644 > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h > @@ -1192,5 +1192,12 @@ static inline const int pud_pfn(pud_t pud) > BUILD_BUG(); > return 0; > } > + > +#ifdef CONFIG_STRICT_KERNEL_RWX > +void mark_initmem_nx(void); > +#else > +static inline void mark_initmem_nx(void) { } > +#endif > + Why do we want to limit that to CONFIG_STRICT_KERNEL_RWX ? Only the kernel text is marked X, even without CONFIG_STRICT_KERNEL_RWX (at least on PPC32), so I believe we should clear X on init text in any case, shouldn't we ? Christophe > #endif /* __ASSEMBLY__ */ > #endif /* _ASM_POWERPC_BOOK3S_64_PGTABLE_H_ */ > diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h > index 487709ff6875..544440b5aff3 100644 > --- a/arch/powerpc/include/asm/book3s/64/radix.h > +++ b/arch/powerpc/include/asm/book3s/64/radix.h > @@ -118,6 +118,7 @@ > > #ifdef CONFIG_STRICT_KERNEL_RWX > extern void radix__mark_rodata_ro(void); > +extern void radix__mark_initmem_nx(void); > #endif > > static inline unsigned long __radix_pte_update(pte_t *ptep, unsigned long clr, > diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c > index 8541f18694a4..46b4e67d2372 100644 > --- a/arch/powerpc/mm/mem.c > +++ b/arch/powerpc/mm/mem.c > @@ -402,6 +402,7 @@ void __init mem_init(void) > void free_initmem(void) > { > ppc_md.progress = ppc_printk_progress; > + mark_initmem_nx(); > free_initmem_default(POISON_FREE_INITMEM); > } > > diff --git a/arch/powerpc/mm/pgtable-hash64.c b/arch/powerpc/mm/pgtable-hash64.c > index 73019c52141f..443a2c66a304 100644 > --- a/arch/powerpc/mm/pgtable-hash64.c > +++ b/arch/powerpc/mm/pgtable-hash64.c > @@ -460,4 +460,16 @@ void hash__mark_rodata_ro(void) > > WARN_ON(!hash__change_memory_range(start, end, PP_RXXX)); > } > + > +void hash__mark_initmem_nx(void) > +{ > + unsigned long start, end, pp; > + > + start = (unsigned long)__init_begin; > + end = (unsigned long)__init_end; > + > + pp = htab_convert_pte_flags(pgprot_val(PAGE_KERNEL)); > + > + WARN_ON(!hash__change_memory_range(start, end, pp)); > +} > #endif > diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c > index 336e52ec652c..5cc50d47ce3f 100644 > --- a/arch/powerpc/mm/pgtable-radix.c > +++ b/arch/powerpc/mm/pgtable-radix.c > @@ -162,6 +162,14 @@ void radix__mark_rodata_ro(void) > > radix__change_memory_range(start, end, _PAGE_WRITE); > } > + > +void radix__mark_initmem_nx(void) > +{ > + unsigned long start = (unsigned long)__init_begin; > + unsigned long end = (unsigned long)__init_end; > + > + radix__change_memory_range(start, end, _PAGE_EXEC); > +} > #endif /* CONFIG_STRICT_KERNEL_RWX */ > > static inline void __meminit print_mapping(unsigned long start, > diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c > index 5c0b795d656c..0736e94c7615 100644 > --- a/arch/powerpc/mm/pgtable_64.c > +++ b/arch/powerpc/mm/pgtable_64.c > @@ -505,4 +505,12 @@ void mark_rodata_ro(void) > else > hash__mark_rodata_ro(); > } > + > +void mark_initmem_nx(void) > +{ > + if (radix_enabled()) > + radix__mark_initmem_nx(); > + else > + hash__mark_initmem_nx(); > +} > #endif >
Christophe LEROY <christophe.leroy@c-s.fr> writes: > Le 14/07/2017 à 08:51, Michael Ellerman a écrit : >> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h >> index c0737c86a362..3d562b210c65 100644 >> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >> @@ -1192,5 +1192,12 @@ static inline const int pud_pfn(pud_t pud) >> BUILD_BUG(); >> return 0; >> } >> + >> +#ifdef CONFIG_STRICT_KERNEL_RWX >> +void mark_initmem_nx(void); >> +#else >> +static inline void mark_initmem_nx(void) { } >> +#endif >> + > > Why do we want to limit that to CONFIG_STRICT_KERNEL_RWX ? > Only the kernel text is marked X, even without CONFIG_STRICT_KERNEL_RWX > (at least on PPC32), so I believe we should clear X on init text in any > case, shouldn't we ? You're right, but .. On 64-bit when STRICT_KERNEL_RWX=n we make no effort to ensure the start/end of the init text is on a page boundary. eg. on 64-bit hash we will typically use a 16M page to map the whole kernel, text/data/init_text/etc. So yes we *should* always mark it no-execute but in practice we can't because it's not page aligned. But if that's different on (some?) 32-bit then we could introduce a new CONFIG symbol that is enabled in the right cases. cheers
Le 09/08/2017 à 04:29, Michael Ellerman a écrit : > Christophe LEROY <christophe.leroy@c-s.fr> writes: >> Le 14/07/2017 à 08:51, Michael Ellerman a écrit : >>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h >>> index c0737c86a362..3d562b210c65 100644 >>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >>> @@ -1192,5 +1192,12 @@ static inline const int pud_pfn(pud_t pud) >>> BUILD_BUG(); >>> return 0; >>> } >>> + >>> +#ifdef CONFIG_STRICT_KERNEL_RWX >>> +void mark_initmem_nx(void); >>> +#else >>> +static inline void mark_initmem_nx(void) { } >>> +#endif >>> + >> >> Why do we want to limit that to CONFIG_STRICT_KERNEL_RWX ? >> Only the kernel text is marked X, even without CONFIG_STRICT_KERNEL_RWX >> (at least on PPC32), so I believe we should clear X on init text in any >> case, shouldn't we ? > > You're right, but .. > > On 64-bit when STRICT_KERNEL_RWX=n we make no effort to ensure the > start/end of the init text is on a page boundary. > > eg. on 64-bit hash we will typically use a 16M page to map the whole > kernel, text/data/init_text/etc. Some of the 32 bit also use some huge mapings like BATs or large pages, in which case it is pointless but not harmfull to fix the page tables anyway. At least it is correct for the ones that use regular pages, and kernel can also be started with nobats or noltlbs at command line, in which case it is usefull to have the page tables correct. > > So yes we *should* always mark it no-execute but in practice we can't > because it's not page aligned. On 32 bit it seems to always be aligned to the normal page size, so no problem. > > But if that's different on (some?) 32-bit then we could introduce a new > CONFIG symbol that is enabled in the right cases. For the time being, I have added an "|| CONFIG_PPC32 " on the ifdef, is that OK ? See https://patchwork.ozlabs.org/patch/796625/ Christophe
Christophe LEROY <christophe.leroy@c-s.fr> writes: ... > At least it is correct for the ones that use regular pages, and kernel > can also be started with nobats or noltlbs at command line, in which > case it is usefull to have the page tables correct. Yep OK. >> So yes we *should* always mark it no-execute but in practice we can't >> because it's not page aligned. > > On 32 bit it seems to always be aligned to the normal page size, so no > problem. > >> But if that's different on (some?) 32-bit then we could introduce a new >> CONFIG symbol that is enabled in the right cases. > > For the time being, I have added an "|| CONFIG_PPC32 " on the ifdef, is > that OK ? > See https://patchwork.ozlabs.org/patch/796625/ Yeah looks fine. cheers
diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h index 0ce513f2926f..36fc7bfe9e11 100644 --- a/arch/powerpc/include/asm/book3s/64/hash.h +++ b/arch/powerpc/include/asm/book3s/64/hash.h @@ -91,6 +91,7 @@ static inline int hash__pgd_bad(pgd_t pgd) } #ifdef CONFIG_STRICT_KERNEL_RWX extern void hash__mark_rodata_ro(void); +extern void hash__mark_initmem_nx(void); #endif extern void hpte_need_flush(struct mm_struct *mm, unsigned long addr, diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h index c0737c86a362..3d562b210c65 100644 --- a/arch/powerpc/include/asm/book3s/64/pgtable.h +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h @@ -1192,5 +1192,12 @@ static inline const int pud_pfn(pud_t pud) BUILD_BUG(); return 0; } + +#ifdef CONFIG_STRICT_KERNEL_RWX +void mark_initmem_nx(void); +#else +static inline void mark_initmem_nx(void) { } +#endif + #endif /* __ASSEMBLY__ */ #endif /* _ASM_POWERPC_BOOK3S_64_PGTABLE_H_ */ diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h index 487709ff6875..544440b5aff3 100644 --- a/arch/powerpc/include/asm/book3s/64/radix.h +++ b/arch/powerpc/include/asm/book3s/64/radix.h @@ -118,6 +118,7 @@ #ifdef CONFIG_STRICT_KERNEL_RWX extern void radix__mark_rodata_ro(void); +extern void radix__mark_initmem_nx(void); #endif static inline unsigned long __radix_pte_update(pte_t *ptep, unsigned long clr, diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index 8541f18694a4..46b4e67d2372 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -402,6 +402,7 @@ void __init mem_init(void) void free_initmem(void) { ppc_md.progress = ppc_printk_progress; + mark_initmem_nx(); free_initmem_default(POISON_FREE_INITMEM); } diff --git a/arch/powerpc/mm/pgtable-hash64.c b/arch/powerpc/mm/pgtable-hash64.c index 73019c52141f..443a2c66a304 100644 --- a/arch/powerpc/mm/pgtable-hash64.c +++ b/arch/powerpc/mm/pgtable-hash64.c @@ -460,4 +460,16 @@ void hash__mark_rodata_ro(void) WARN_ON(!hash__change_memory_range(start, end, PP_RXXX)); } + +void hash__mark_initmem_nx(void) +{ + unsigned long start, end, pp; + + start = (unsigned long)__init_begin; + end = (unsigned long)__init_end; + + pp = htab_convert_pte_flags(pgprot_val(PAGE_KERNEL)); + + WARN_ON(!hash__change_memory_range(start, end, pp)); +} #endif diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c index 336e52ec652c..5cc50d47ce3f 100644 --- a/arch/powerpc/mm/pgtable-radix.c +++ b/arch/powerpc/mm/pgtable-radix.c @@ -162,6 +162,14 @@ void radix__mark_rodata_ro(void) radix__change_memory_range(start, end, _PAGE_WRITE); } + +void radix__mark_initmem_nx(void) +{ + unsigned long start = (unsigned long)__init_begin; + unsigned long end = (unsigned long)__init_end; + + radix__change_memory_range(start, end, _PAGE_EXEC); +} #endif /* CONFIG_STRICT_KERNEL_RWX */ static inline void __meminit print_mapping(unsigned long start, diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c index 5c0b795d656c..0736e94c7615 100644 --- a/arch/powerpc/mm/pgtable_64.c +++ b/arch/powerpc/mm/pgtable_64.c @@ -505,4 +505,12 @@ void mark_rodata_ro(void) else hash__mark_rodata_ro(); } + +void mark_initmem_nx(void) +{ + if (radix_enabled()) + radix__mark_initmem_nx(); + else + hash__mark_initmem_nx(); +} #endif
Currently even with STRICT_KERNEL_RWX we leave the __init text marked executable after init, which is bad. Add a hook to mark it NX (no-execute) before we free it, and implement it for radix and hash. Note that we use __init_end as the end address, not _einittext, because overlaps_kernel_text() uses __init_end, because there are additional executable sections other than .init.text between __init_begin and __init_end. Tested on radix and hash with: 0:mon> p $__init_begin *** 400 exception occurred Fixes: 1e0fc9d1eb2b ("powerpc/Kconfig: Enable STRICT_KERNEL_RWX for some configs") Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/include/asm/book3s/64/hash.h | 1 + arch/powerpc/include/asm/book3s/64/pgtable.h | 7 +++++++ arch/powerpc/include/asm/book3s/64/radix.h | 1 + arch/powerpc/mm/mem.c | 1 + arch/powerpc/mm/pgtable-hash64.c | 12 ++++++++++++ arch/powerpc/mm/pgtable-radix.c | 8 ++++++++ arch/powerpc/mm/pgtable_64.c | 8 ++++++++ 7 files changed, 38 insertions(+)