Re: [PATCH 2/2] ARM: mm: make text and rodata read-only

[Rabin Vincent]

On Thu, Apr 03, 2014 at 07:15:19PM -0700, Kees Cook wrote:
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index 34e56647dcee..4ae343c1e2a3 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/ftrace.h>
>  #include <linux/uaccess.h>
> +#include <linux/stop_machine.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/opcodes.h>
> @@ -34,6 +35,22 @@
>  
>  #define	OLD_NOP		0xe1a00000	/* mov r0, r0 */
>  
> +static int __ftrace_modify_code(void *data)

This is in the CONFIG_OLD_MCOUNT ifdef, but should be in the outer ifdef
(CONFIG_DYNAMIC_FTRACE) instead, otherwise it will not get enabled for
for example Thumb-2 kernels.  This was wrong in my example patch too.

> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 8539eb2a01ad..3baac4ad165f 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -681,30 +716,52 @@ static inline bool arch_has_strict_perms(void)
>  	return true;
>  }
>  
> +#define set_section_perms(perms, field)	{				\
> +	size_t i;							\
> +	unsigned long addr;						\
> +									\
> +	if (!arch_has_strict_perms())					\
> +		return;							\
> +									\
> +	for (i = 0; i < ARRAY_SIZE(perms); i++) {			\
> +		if (!IS_ALIGNED(perms[i].start, SECTION_SIZE) ||	\
> +		    !IS_ALIGNED(perms[i].end, SECTION_SIZE)) {		\
> +			pr_err("BUG: section %lx-%lx not aligned to %lx\n", \
> +				perms[i].start, perms[i].end,		\
> +				SECTION_SIZE);				\
> +			continue;					\
> +		}							\
> +									\
> +		for (addr = perms[i].start;				\
> +		     addr < perms[i].end;				\
> +		     addr += SECTION_SIZE)				\
> +			section_update(addr, perms[i].mask,		\
> +				       perms[i].field);			\
> +	}								\
> +}
> +
>  static inline void fix_kernmem_perms(void)
>  {
> -	unsigned long addr;
> -	unsigned int i;
> +	set_section_perms(nx_perms, prot);
> +}
>  
> -	if (!arch_has_strict_perms())
> -		return;
> +#ifdef CONFIG_DEBUG_RODATA
> +void mark_rodata_ro(void)
> +{
> +	set_section_perms(ro_perms, prot);
> +}
>  
> -	for (i = 0; i < ARRAY_SIZE(section_perms); i++) {
> -		if (!IS_ALIGNED(section_perms[i].start, SECTION_SIZE) ||
> -		    !IS_ALIGNED(section_perms[i].end, SECTION_SIZE)) {
> -			pr_err("BUG: section %lx-%lx not aligned to %lx\n",
> -				section_perms[i].start, section_perms[i].end,
> -				SECTION_SIZE);
> -			continue;
> -		}
> +void set_kernel_text_rw(void)
> +{
> +	set_section_perms(ro_perms, clear);
> +}

You need a TLB flush.  I had a flush_tlb_all() in my example patch,
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/244335.html,
but the following is probably nicer (on top of this patch):

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 9bea524..a92c45a 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -741,6 +741,8 @@ static inline bool arch_has_strict_perms(void)
 		     addr += SECTION_SIZE)				\
 			section_update(addr, perms[i].mask,		\
 				       perms[i].field);			\
+									\
+		flush_tlb_kernel_range(perms[i].start, perms[i].end);	\
 	}								\
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/