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

Message ID 1500015083-23511-3-git-send-email-mpe@ellerman.id.au
State Accepted
Commit 029d9252b116fa52a95150819e62af1f6e420fe5
Headers show

Commit Message

Michael Ellerman July 14, 2017, 6:51 a.m.
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(+)

Comments

kbuild test robot July 14, 2017, 11:01 p.m. | #1
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
Christophe LEROY Aug. 2, 2017, 11:01 a.m. | #2
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
>
Michael Ellerman Aug. 9, 2017, 2:29 a.m. | #3
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
Christophe LEROY Aug. 9, 2017, 6:27 a.m. | #4
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
Michael Ellerman Aug. 9, 2017, 1:13 p.m. | #5
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

Patch

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