* [PATCH 2/3] powerpc/mm/hash: Refactor hash__mark_rodata_ro()
2017-07-14 6:51 [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro() Michael Ellerman
@ 2017-07-14 6:51 ` Michael Ellerman
2017-07-18 1:05 ` Balbir Singh
2017-07-14 6:51 ` [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y Michael Ellerman
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Michael Ellerman @ 2017-07-14 6:51 UTC (permalink / raw)
To: linuxppc-dev
Move the core logic into a helper, so we can use it for changing other
permissions.
We also change the logic to align start down, and end up. This means
calling the function with a range will expand that range to be at
least 1 mmu_linear_psize page in size. We need that so we can use it
on __init_begin ... __init_end which is not a full page in size.
This should always work for _stext/__init_begin, because we align
__init_begin to _stext + 16M in the linker script.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/mm/pgtable-hash64.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/mm/pgtable-hash64.c b/arch/powerpc/mm/pgtable-hash64.c
index 188b4107584d..73019c52141f 100644
--- a/arch/powerpc/mm/pgtable-hash64.c
+++ b/arch/powerpc/mm/pgtable-hash64.c
@@ -425,33 +425,39 @@ int hash__has_transparent_hugepage(void)
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
#ifdef CONFIG_STRICT_KERNEL_RWX
-void hash__mark_rodata_ro(void)
+static bool hash__change_memory_range(unsigned long start, unsigned long end,
+ unsigned long newpp)
{
- unsigned long start = (unsigned long)_stext;
- unsigned long end = (unsigned long)__init_begin;
unsigned long idx;
unsigned int step, shift;
- unsigned long newpp = PP_RXXX;
shift = mmu_psize_defs[mmu_linear_psize].shift;
step = 1 << shift;
- start = ((start + step - 1) >> shift) << shift;
- end = (end >> shift) << shift;
+ start = ALIGN_DOWN(start, step);
+ end = ALIGN(end, step); // aligns up
- pr_devel("marking ro start %lx, end %lx, step %x\n",
- start, end, step);
+ if (start >= end)
+ return false;
- if (start == end) {
- pr_warn("could not set rodata ro, relocate the start"
- " of the kernel to a 0x%x boundary\n", step);
- return;
- }
+ pr_debug("Changing page protection on range 0x%lx-0x%lx, to 0x%lx, step 0x%x\n",
+ start, end, newpp, step);
for (idx = start; idx < end; idx += step)
/* Not sure if we can do much with the return value */
mmu_hash_ops.hpte_updateboltedpp(newpp, idx, mmu_linear_psize,
mmu_kernel_ssize);
+ return true;
+}
+
+void hash__mark_rodata_ro(void)
+{
+ unsigned long start, end;
+
+ start = (unsigned long)_stext;
+ end = (unsigned long)__init_begin;
+
+ WARN_ON(!hash__change_memory_range(start, end, PP_RXXX));
}
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] powerpc/mm/hash: Refactor hash__mark_rodata_ro()
2017-07-14 6:51 ` [PATCH 2/3] powerpc/mm/hash: Refactor hash__mark_rodata_ro() Michael Ellerman
@ 2017-07-18 1:05 ` Balbir Singh
0 siblings, 0 replies; 12+ messages in thread
From: Balbir Singh @ 2017-07-18 1:05 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Fri, Jul 14, 2017 at 4:51 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Move the core logic into a helper, so we can use it for changing other
> permissions.
>
> We also change the logic to align start down, and end up. This means
> calling the function with a range will expand that range to be at
> least 1 mmu_linear_psize page in size. We need that so we can use it
> on __init_begin ... __init_end which is not a full page in size.
>
> This should always work for _stext/__init_begin, because we align
> __init_begin to _stext + 16M in the linker script.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
Reviewed-by: Balbir Singh <bsingharora@gmail.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-07-14 6:51 [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro() Michael Ellerman
2017-07-14 6:51 ` [PATCH 2/3] powerpc/mm/hash: Refactor hash__mark_rodata_ro() Michael Ellerman
@ 2017-07-14 6:51 ` Michael Ellerman
2017-07-14 23:01 ` kbuild test robot
2017-08-02 11:01 ` Christophe LEROY
2017-07-18 1:04 ` [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro() Balbir Singh
2017-07-21 11:13 ` [1/3] " Michael Ellerman
3 siblings, 2 replies; 12+ messages in thread
From: Michael Ellerman @ 2017-07-14 6:51 UTC (permalink / raw)
To: linuxppc-dev
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
+
#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
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-07-14 6:51 ` [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y Michael Ellerman
@ 2017-07-14 23:01 ` kbuild test robot
2017-07-18 9:54 ` Michael Ellerman
2017-08-02 11:01 ` Christophe LEROY
1 sibling, 1 reply; 12+ messages in thread
From: kbuild test robot @ 2017-07-14 23:01 UTC (permalink / raw)
To: Michael Ellerman; +Cc: kbuild-all, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6292 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-07-14 23:01 ` kbuild test robot
@ 2017-07-18 9:54 ` Michael Ellerman
0 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2017-07-18 9:54 UTC (permalink / raw)
To: linuxppc-dev
kbuild test robot <lkp@intel.com> writes:
> 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
Gah, 32-bit.
Fixed with:
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 3d562b210c65..d1da415e283c 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -1193,11 +1193,5 @@ static inline const int pud_pfn(pud_t pud)
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/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index dd01212935ac..afae9a336136 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -80,6 +80,13 @@ unsigned long vmalloc_to_phys(void *vmalloc_addr);
void pgtable_cache_add(unsigned shift, void (*ctor)(void *));
void pgtable_cache_init(void);
+
+#ifdef CONFIG_STRICT_KERNEL_RWX
+void mark_initmem_nx(void);
+#else
+static inline void mark_initmem_nx(void) { }
+#endif
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_PGTABLE_H */
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-07-14 6:51 ` [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y Michael Ellerman
2017-07-14 23:01 ` kbuild test robot
@ 2017-08-02 11:01 ` Christophe LEROY
2017-08-09 2:29 ` Michael Ellerman
1 sibling, 1 reply; 12+ messages in thread
From: Christophe LEROY @ 2017-08-02 11:01 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-08-02 11:01 ` Christophe LEROY
@ 2017-08-09 2:29 ` Michael Ellerman
2017-08-09 6:27 ` Christophe LEROY
0 siblings, 1 reply; 12+ messages in thread
From: Michael Ellerman @ 2017-08-09 2:29 UTC (permalink / raw)
To: Christophe LEROY, linuxppc-dev
Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 14/07/2017 =C3=A0 08:51, Michael Ellerman a =C3=A9crit :
>> 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=20
> (at least on PPC32), so I believe we should clear X on init text in any=20
> case, shouldn't we ?
You're right, but ..
On 64-bit when STRICT_KERNEL_RWX=3Dn 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-08-09 2:29 ` Michael Ellerman
@ 2017-08-09 6:27 ` Christophe LEROY
2017-08-09 13:13 ` Michael Ellerman
0 siblings, 1 reply; 12+ messages in thread
From: Christophe LEROY @ 2017-08-09 6:27 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y
2017-08-09 6:27 ` Christophe LEROY
@ 2017-08-09 13:13 ` Michael Ellerman
0 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2017-08-09 13:13 UTC (permalink / raw)
To: Christophe LEROY, linuxppc-dev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro()
2017-07-14 6:51 [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro() Michael Ellerman
2017-07-14 6:51 ` [PATCH 2/3] powerpc/mm/hash: Refactor hash__mark_rodata_ro() Michael Ellerman
2017-07-14 6:51 ` [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y Michael Ellerman
@ 2017-07-18 1:04 ` Balbir Singh
2017-07-21 11:13 ` [1/3] " Michael Ellerman
3 siblings, 0 replies; 12+ messages in thread
From: Balbir Singh @ 2017-07-18 1:04 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Fri, Jul 14, 2017 at 4:51 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Move the core logic into a helper, so we can use it for changing permissions
> other than _PAGE_WRITE.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/mm/pgtable-radix.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 8c13e4282308..336e52ec652c 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -112,10 +112,9 @@ int radix__map_kernel_page(unsigned long ea, unsigned long pa,
> }
>
> #ifdef CONFIG_STRICT_KERNEL_RWX
> -void radix__mark_rodata_ro(void)
> +void radix__change_memory_range(unsigned long start, unsigned long end,
> + unsigned long clear)
> {
> - unsigned long start = (unsigned long)_stext;
> - unsigned long end = (unsigned long)__init_begin;
> unsigned long idx;
> pgd_t *pgdp;
> pud_t *pudp;
> @@ -125,7 +124,8 @@ void radix__mark_rodata_ro(void)
> start = ALIGN_DOWN(start, PAGE_SIZE);
> end = PAGE_ALIGN(end); // aligns up
>
> - pr_devel("marking ro start %lx, end %lx\n", start, end);
> + pr_debug("Changing flags on range %lx-%lx removing 0x%lx\n",
> + start, end, clear);
>
> for (idx = start; idx < end; idx += PAGE_SIZE) {
> pgdp = pgd_offset_k(idx);
> @@ -147,11 +147,21 @@ void radix__mark_rodata_ro(void)
> if (!ptep)
> continue;
> update_the_pte:
> - radix__pte_update(&init_mm, idx, ptep, _PAGE_WRITE, 0, 0);
> + radix__pte_update(&init_mm, idx, ptep, clear, 0, 0);
> }
>
> radix__flush_tlb_kernel_range(start, end);
> }
> +
> +void radix__mark_rodata_ro(void)
> +{
> + unsigned long start, end;
> +
> + start = (unsigned long)_stext;
> + end = (unsigned long)__init_begin;
> +
> + radix__change_memory_range(start, end, _PAGE_WRITE);
> +}
> #endif /* CONFIG_STRICT_KERNEL_RWX */
>
Looks good, I had something similar for the patches to enable
relocation, but that's a
a larger chunk.
Reviewed-by: Balbir Singh <bsingharora@gmail.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro()
2017-07-14 6:51 [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro() Michael Ellerman
` (2 preceding siblings ...)
2017-07-18 1:04 ` [PATCH 1/3] powerpc/mm/radix: Refactor radix__mark_rodata_ro() Balbir Singh
@ 2017-07-21 11:13 ` Michael Ellerman
3 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2017-07-21 11:13 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
On Fri, 2017-07-14 at 06:51:21 UTC, Michael Ellerman wrote:
> Move the core logic into a helper, so we can use it for changing permissions
> other than _PAGE_WRITE.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Series applied to powerpc fixes.
https://git.kernel.org/powerpc/c/b134bd90286dc9f2952c35a91ab405
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread