Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Michael Ellerman]

Christophe LEROY  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

Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Christophe LEROY]

Le 09/08/2017 à 04:29, Michael Ellerman a écrit :

Christophe LEROY  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

Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Michael Ellerman]

Christophe LEROY  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

Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Christophe LEROY]

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 
---
  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..50b5aff3 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
+   

Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Michael Ellerman]

kbuild test robot  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 */

Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[kbuild test robot]

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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Michael Ellerman]

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 
---
 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..50b5aff3 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