[PATCH] ARM: mm: add testcases for RODATA

[PATCH] ARM: mm: add testcases for RODATA

[Jinbum Park]

This patch adds testcases for the CONFIG_DEBUG_RODATA option.
It's similar to x86's testcases.
It tests read-only mapped data and page-size aligned rodata section.

Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
---
 arch/arm/Kconfig.debug            |  5 +++
 arch/arm/include/asm/cacheflush.h | 10 +++++
 arch/arm/mm/Makefile              |  1 +
 arch/arm/mm/init.c                |  6 +++
 arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 arch/arm/mm/test_rodata.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index d83f7c3..511e5e1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
 	  against certain classes of kernel exploits.
 	  If in doubt, say "N".
 
+config DEBUG_RODATA_TEST
+	bool "Testcase for the marking rodata read-only"
+	---help---
+	  This option enables a testcase for the setting rodata read-only.
+
 source "drivers/hwtracing/coresight/Kconfig"
 
 endmenu
diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index bdd283b..741e2e8 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
 static inline void set_kernel_text_ro(void) { }
 #endif
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+extern const int rodata_test_data;
+int rodata_test(void);
+#else
+static inline int rodata_test(void)
+{
+	return 0;
+}
+#endif
+
 void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
 			     void *kaddr, unsigned long len);
 
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index b3dea80..dbb76ff 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -15,6 +15,7 @@ endif
 obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
 obj-$(CONFIG_MODULES)		+= proc-syms.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 4127f57..3c15f3b 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
 int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
+	rodata_test();
 	return 0;
 }
 
@@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
 static inline void fix_kernmem_perms(void) { }
 #endif /* CONFIG_DEBUG_RODATA */
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+const int rodata_test_data = 0xC3;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+#endif
+
 void free_tcmmem(void)
 {
 #ifdef CONFIG_HAVE_TCM
diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
new file mode 100644
index 0000000..133d092
--- /dev/null
+++ b/arch/arm/mm/test_rodata.c
@@ -0,0 +1,79 @@
+/*
+ * test_rodata.c: functional test for mark_rodata_ro function
+ *
+ * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <asm/cacheflush.h>
+#include <asm/sections.h>
+
+int rodata_test(void)
+{
+	unsigned long result;
+	unsigned long start, end;
+
+	/* test 1: read the value */
+	/* If this test fails, some previous testrun has clobbered the state */
+
+	if (!rodata_test_data) {
+		pr_err("rodata_test: test 1 fails (start data)\n");
+		return -ENODEV;
+	}
+
+	/* test 2: write to the variable; this should fault */
+	/*
+	 * If this test fails, we managed to overwrite the data
+	 *
+	 * This is written in assembly to be able to catch the
+	 * exception that is supposed to happen in the correct
+	 * case
+	*/
+
+	result = 1;
+	asm volatile(
+		"0:	str %[zero], [%[rodata_test]]\n"
+		"	mov %[rslt], %[zero]\n"
+		"1:\n"
+		".pushsection .text.fixup,\"ax\"\n"
+		".align 2\n"
+		"2:\n"
+		"b 1b\n"
+		".popsection\n"
+		".pushsection __ex_table,\"a\"\n"
+		".align 3\n"
+		".long 0b, 2b\n"
+		".popsection\n"
+		: [rslt] "=r" (result)
+		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
+	);
+
+	if (!result) {
+		pr_err("rodata_test: test data was not read only\n");
+		return -ENODEV;
+	}
+
+	/* test 3: check the value hasn't changed */
+	/* If this test fails, we managed to overwrite the data */
+	if (!rodata_test_data) {
+		pr_err("rodata_test: Test 3 fails (end data)\n");
+		return -ENODEV;
+	}
+
+	/* test 4: check if the rodata section is 4Kb aligned */
+	start = (unsigned long)__start_rodata;
+	end = (unsigned long)__end_rodata;
+	if (start & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata is not 4k aligned\n");
+		return -ENODEV;
+	}
+	if (end & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata end is not 4k aligned\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
-- 
1.9.1

Re: [PATCH] ARM: mm: add testcases for RODATA

[Mark Rutland]

On Wed, Jan 18, 2017 at 10:53:10PM +0900, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.

I note that LKDTM already has a similar test (though it just has a raw
write, and will crash the kernel).

> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);

This is the only architecture-specific part of the file.

Rather than duplicating the logic from x86, can't we use generic
infrastructure for this part, and move the existing test into a shared
location?

e.g. could we change to KERNEL_DS and use put_user here?

> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

s/4k/page/ in the prints, if this becomes generic.

Thanks,
Mark.

Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 05:53 AM, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.
> 
> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
> ---
>  arch/arm/Kconfig.debug            |  5 +++
>  arch/arm/include/asm/cacheflush.h | 10 +++++
>  arch/arm/mm/Makefile              |  1 +
>  arch/arm/mm/init.c                |  6 +++
>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 arch/arm/mm/test_rodata.c
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..511e5e1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>  	  against certain classes of kernel exploits.
>  	  If in doubt, say "N".
>  
> +config DEBUG_RODATA_TEST
> +	bool "Testcase for the marking rodata read-only"
> +	---help---
> +	  This option enables a testcase for the setting rodata read-only.
> +
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu
> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> index bdd283b..741e2e8 100644
> --- a/arch/arm/include/asm/cacheflush.h
> +++ b/arch/arm/include/asm/cacheflush.h
> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>  static inline void set_kernel_text_ro(void) { }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +extern const int rodata_test_data;
> +int rodata_test(void);
> +#else
> +static inline int rodata_test(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>  			     void *kaddr, unsigned long len);
>  
> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> index b3dea80..dbb76ff 100644
> --- a/arch/arm/mm/Makefile
> +++ b/arch/arm/mm/Makefile
> @@ -15,6 +15,7 @@ endif
>  obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
>  obj-$(CONFIG_MODULES)		+= proc-syms.o
>  obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
> +obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
>  
>  obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
>  obj-$(CONFIG_HIGHMEM)		+= highmem.o
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 4127f57..3c15f3b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>  int __mark_rodata_ro(void *unused)
>  {
>  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> +	rodata_test();

We don't do anything with this return value, should we at least
spit out a warning?

>  	return 0;
>  }
>  
> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>  static inline void fix_kernmem_perms(void) { }
>  #endif /* CONFIG_DEBUG_RODATA */
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +const int rodata_test_data = 0xC3;

This isn't accessed outside of test_rodata.c, it can just
be moved there.

> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +#endif
> +
>  void free_tcmmem(void)
>  {
>  #ifdef CONFIG_HAVE_TCM
> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> new file mode 100644
> index 0000000..133d092
> --- /dev/null
> +++ b/arch/arm/mm/test_rodata.c
> @@ -0,0 +1,79 @@
> +/*
> + * test_rodata.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <asm/cacheflush.h>
> +#include <asm/sections.h>
> +
> +int rodata_test(void)
> +{
> +	unsigned long result;
> +	unsigned long start, end;
> +
> +	/* test 1: read the value */
> +	/* If this test fails, some previous testrun has clobbered the state */
> +
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: test 1 fails (start data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 2: write to the variable; this should fault */
> +	/*
> +	 * If this test fails, we managed to overwrite the data
> +	 *
> +	 * This is written in assembly to be able to catch the
> +	 * exception that is supposed to happen in the correct
> +	 * case
> +	*/
> +
> +	result = 1;
> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);
> +
> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}

I'm confused why we are checking this again when we have the result
check above. Is there a case where we would still have result = 1
but rodata_test_data overwritten?

> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

Why does this need to be page aligned (yes, I know why but this
test case does not make it clear)

Better yet, this should be a build time assertion not a runtime
one.

> +
> +	return 0;
> +}
> 

As Mark mentioned, this is possibly redundant with LKDTM. It
would be good to explain what benefit this is bringing in addition
to LKDTM.

Thanks,
Laura

Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 11:20 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
>> It's similar to x86's testcases.
>> It tests read-only mapped data and page-size aligned rodata section.
>>
>> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
>> ---
>>  arch/arm/Kconfig.debug            |  5 +++
>>  arch/arm/include/asm/cacheflush.h | 10 +++++
>>  arch/arm/mm/Makefile              |  1 +
>>  arch/arm/mm/init.c                |  6 +++
>>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 102 insertions(+)
>>  create mode 100644 arch/arm/mm/test_rodata.c
>>
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index d83f7c3..511e5e1 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>>         against certain classes of kernel exploits.
>>         If in doubt, say "N".
>>
>> +config DEBUG_RODATA_TEST
>> +     bool "Testcase for the marking rodata read-only"
>> +     ---help---
>> +       This option enables a testcase for the setting rodata read-only.
>> +
>>  source "drivers/hwtracing/coresight/Kconfig"
>>
>>  endmenu
>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> index bdd283b..741e2e8 100644
>> --- a/arch/arm/include/asm/cacheflush.h
>> +++ b/arch/arm/include/asm/cacheflush.h
>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>  static inline void set_kernel_text_ro(void) { }
>>  #endif
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +extern const int rodata_test_data;
>> +int rodata_test(void);
>> +#else
>> +static inline int rodata_test(void)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>>                            void *kaddr, unsigned long len);
>>
>> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
>> index b3dea80..dbb76ff 100644
>> --- a/arch/arm/mm/Makefile
>> +++ b/arch/arm/mm/Makefile
>> @@ -15,6 +15,7 @@ endif
>>  obj-$(CONFIG_ARM_PTDUMP)     += dump.o
>>  obj-$(CONFIG_MODULES)                += proc-syms.o
>>  obj-$(CONFIG_DEBUG_VIRTUAL)  += physaddr.o
>> +obj-$(CONFIG_DEBUG_RODATA_TEST)      += test_rodata.o
>>
>>  obj-$(CONFIG_ALIGNMENT_TRAP) += alignment.o
>>  obj-$(CONFIG_HIGHMEM)                += highmem.o
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index 4127f57..3c15f3b 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>>  int __mark_rodata_ro(void *unused)
>>  {
>>       update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> +     rodata_test();
>
> We don't do anything with this return value, should we at least
> spit out a warning?
>
>>       return 0;
>>  }
>>
>> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>>  static inline void fix_kernmem_perms(void) { }
>>  #endif /* CONFIG_DEBUG_RODATA */
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +const int rodata_test_data = 0xC3;
>
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.
>
>> +EXPORT_SYMBOL_GPL(rodata_test_data);
>> +#endif
>> +
>>  void free_tcmmem(void)
>>  {
>>  #ifdef CONFIG_HAVE_TCM
>> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> new file mode 100644
>> index 0000000..133d092
>> --- /dev/null
>> +++ b/arch/arm/mm/test_rodata.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * test_rodata.c: functional test for mark_rodata_ro function
>> + *
>> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; version 2
>> + * of the License.
>> + */
>> +#include <asm/cacheflush.h>
>> +#include <asm/sections.h>
>> +
>> +int rodata_test(void)
>> +{
>> +     unsigned long result;
>> +     unsigned long start, end;
>> +
>> +     /* test 1: read the value */
>> +     /* If this test fails, some previous testrun has clobbered the state */
>> +
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: test 1 fails (start data)\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 2: write to the variable; this should fault */
>> +     /*
>> +      * If this test fails, we managed to overwrite the data
>> +      *
>> +      * This is written in assembly to be able to catch the
>> +      * exception that is supposed to happen in the correct
>> +      * case
>> +     */
>> +
>> +     result = 1;
>> +     asm volatile(
>> +             "0:     str %[zero], [%[rodata_test]]\n"
>> +             "       mov %[rslt], %[zero]\n"
>> +             "1:\n"
>> +             ".pushsection .text.fixup,\"ax\"\n"
>> +             ".align 2\n"
>> +             "2:\n"
>> +             "b 1b\n"
>> +             ".popsection\n"
>> +             ".pushsection __ex_table,\"a\"\n"
>> +             ".align 3\n"
>> +             ".long 0b, 2b\n"
>> +             ".popsection\n"
>> +             : [rslt] "=r" (result)
>> +             : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> +     );
>> +
>> +     if (!result) {
>> +             pr_err("rodata_test: test data was not read only\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 3: check the value hasn't changed */
>> +     /* If this test fails, we managed to overwrite the data */
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: Test 3 fails (end data)\n");
>> +             return -ENODEV;
>> +     }
>
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?
>
>> +
>> +     /* test 4: check if the rodata section is 4Kb aligned */
>> +     start = (unsigned long)__start_rodata;
>> +     end = (unsigned long)__end_rodata;
>> +     if (start & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>> +     if (end & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata end is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>
> Why does this need to be page aligned (yes, I know why but this
> test case does not make it clear)
>
> Better yet, this should be a build time assertion not a runtime
> one.
>
>> +
>> +     return 0;
>> +}
>>
>
> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

The only thing I could think of would be to make failures block the
boot. (Though that would mean changing x86 too.) That's kind of like
the W^X test, which spits out a BUG IIRC.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> > index bdd283b..741e2e8 100644
> > --- a/arch/arm/include/asm/cacheflush.h
> > +++ b/arch/arm/include/asm/cacheflush.h
> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
> >  static inline void set_kernel_text_ro(void) { }
> >  #endif
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +extern const int rodata_test_data;
> > +int rodata_test(void);
> > +#else
> > +static inline int rodata_test(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +

I don't see why this needs to be in cacheflush.h - it doesn't seem to
have anything to do with cache flushing, and placing it in here means
that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
the entire kernel gets rebuilt.  Please put it in a separate header
file.

> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 4127f57..3c15f3b 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
> >  int __mark_rodata_ro(void *unused)
> >  {
> >  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> > +	rodata_test();
> 
> We don't do anything with this return value, should we at least
> spit out a warning?
> 
> >  	return 0;
> >  }
> >  
> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
> >  static inline void fix_kernmem_perms(void) { }
> >  #endif /* CONFIG_DEBUG_RODATA */
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +const int rodata_test_data = 0xC3;
> 
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.

I think the intention was to place it in some .c file which gets built
into the kernel, rather than a module, so testing whether read-only
data in the kernel image is read-only.

If it's placed in a module, then you're only checking that read-only
data in the module is read-only (which is another test which should
be done!)

In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
rather it went in its own separate file.

> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> > new file mode 100644
> > index 0000000..133d092
> > --- /dev/null
> > +++ b/arch/arm/mm/test_rodata.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * test_rodata.c: functional test for mark_rodata_ro function
> > + *
> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +#include <asm/cacheflush.h>
> > +#include <asm/sections.h>
> > +
> > +int rodata_test(void)
> > +{
> > +	unsigned long result;
> > +	unsigned long start, end;
> > +
> > +	/* test 1: read the value */
> > +	/* If this test fails, some previous testrun has clobbered the state */
> > +
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: test 1 fails (start data)\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 2: write to the variable; this should fault */
> > +	/*
> > +	 * If this test fails, we managed to overwrite the data
> > +	 *
> > +	 * This is written in assembly to be able to catch the
> > +	 * exception that is supposed to happen in the correct
> > +	 * case
> > +	*/
> > +
> > +	result = 1;
> > +	asm volatile(
> > +		"0:	str %[zero], [%[rodata_test]]\n"
> > +		"	mov %[rslt], %[zero]\n"
> > +		"1:\n"
> > +		".pushsection .text.fixup,\"ax\"\n"
> > +		".align 2\n"
> > +		"2:\n"
> > +		"b 1b\n"
> > +		".popsection\n"
> > +		".pushsection __ex_table,\"a\"\n"
> > +		".align 3\n"
> > +		".long 0b, 2b\n"
> > +		".popsection\n"
> > +		: [rslt] "=r" (result)
> > +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> > +	);
> > +
> > +	if (!result) {
> > +		pr_err("rodata_test: test data was not read only\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 3: check the value hasn't changed */
> > +	/* If this test fails, we managed to overwrite the data */
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: Test 3 fails (end data)\n");
> > +		return -ENODEV;
> > +	}
> 
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?

Seems sensible when you consider that "result" tests that _a_ fault
happened.  Verifying that the data wasn't changed seems like a belt
and braces approach, which ensures that the location really has not
been modified.

IOW, the test is "make sure that read-only data is not modified" not
"make sure that writing to read-only data causes a fault".  They're
two subtly different tests.

> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

Finding https://lwn.net/Articles/198690/ and github links, it doesn't
seem obvious that LKDTM actually does this.  It seems more focused on
creating crashdumps than testing that (in this instance) write
protection works - and it seems to be a destructive test.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 2:36 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> > index bdd283b..741e2e8 100644
>> > --- a/arch/arm/include/asm/cacheflush.h
>> > +++ b/arch/arm/include/asm/cacheflush.h
>> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>> >  static inline void set_kernel_text_ro(void) { }
>> >  #endif
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +extern const int rodata_test_data;
>> > +int rodata_test(void);
>> > +#else
>> > +static inline int rodata_test(void)
>> > +{
>> > +   return 0;
>> > +}
>> > +#endif
>> > +
>
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
>
>> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> > index 4127f57..3c15f3b 100644
>> > --- a/arch/arm/mm/init.c
>> > +++ b/arch/arm/mm/init.c
>> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>> >  int __mark_rodata_ro(void *unused)
>> >  {
>> >     update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> > +   rodata_test();
>>
>> We don't do anything with this return value, should we at least
>> spit out a warning?
>>
>> >     return 0;
>> >  }
>> >
>> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>> >  static inline void fix_kernmem_perms(void) { }
>> >  #endif /* CONFIG_DEBUG_RODATA */
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +const int rodata_test_data = 0xC3;

Oh, I missed this the first time: this may be misleading: 0xc3 is x86
"ret", and is used on x86 for it's test_nx.c file. (Which, IIRC,
hasn't worked correctly for years now since the exception tables were
made relative on x86. Getting this fixed too would be nice!)

>> This isn't accessed outside of test_rodata.c, it can just
>> be moved there.

Just for background, on x86 it's referenced by both test_rodata.c and test_nx.c.

> I think the intention was to place it in some .c file which gets built
> into the kernel, rather than a module, so testing whether read-only
> data in the kernel image is read-only.
>
> If it's placed in a module, then you're only checking that read-only
> data in the module is read-only (which is another test which should
> be done!)
>
> In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
> rather it went in its own separate file.
>
>> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> > new file mode 100644
>> > index 0000000..133d092
>> > --- /dev/null
>> > +++ b/arch/arm/mm/test_rodata.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * test_rodata.c: functional test for mark_rodata_ro function
>> > + *
>> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <asm/cacheflush.h>
>> > +#include <asm/sections.h>
>> > +
>> > +int rodata_test(void)
>> > +{
>> > +   unsigned long result;
>> > +   unsigned long start, end;
>> > +
>> > +   /* test 1: read the value */
>> > +   /* If this test fails, some previous testrun has clobbered the state */
>> > +
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: test 1 fails (start data)\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 2: write to the variable; this should fault */
>> > +   /*
>> > +    * If this test fails, we managed to overwrite the data
>> > +    *
>> > +    * This is written in assembly to be able to catch the
>> > +    * exception that is supposed to happen in the correct
>> > +    * case
>> > +   */
>> > +
>> > +   result = 1;
>> > +   asm volatile(
>> > +           "0:     str %[zero], [%[rodata_test]]\n"
>> > +           "       mov %[rslt], %[zero]\n"
>> > +           "1:\n"
>> > +           ".pushsection .text.fixup,\"ax\"\n"
>> > +           ".align 2\n"
>> > +           "2:\n"
>> > +           "b 1b\n"
>> > +           ".popsection\n"
>> > +           ".pushsection __ex_table,\"a\"\n"
>> > +           ".align 3\n"
>> > +           ".long 0b, 2b\n"
>> > +           ".popsection\n"
>> > +           : [rslt] "=r" (result)
>> > +           : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> > +   );
>> > +
>> > +   if (!result) {
>> > +           pr_err("rodata_test: test data was not read only\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 3: check the value hasn't changed */
>> > +   /* If this test fails, we managed to overwrite the data */
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: Test 3 fails (end data)\n");
>> > +           return -ENODEV;
>> > +   }
>>
>> I'm confused why we are checking this again when we have the result
>> check above. Is there a case where we would still have result = 1
>> but rodata_test_data overwritten?
>
> Seems sensible when you consider that "result" tests that _a_ fault
> happened.  Verifying that the data wasn't changed seems like a belt
> and braces approach, which ensures that the location really has not
> been modified.
>
> IOW, the test is "make sure that read-only data is not modified" not
> "make sure that writing to read-only data causes a fault".  They're
> two subtly different tests.
>
>> As Mark mentioned, this is possibly redundant with LKDTM. It
>> would be good to explain what benefit this is bringing in addition
>> to LKDTM.
>
> Finding https://lwn.net/Articles/198690/ and github links, it doesn't
> seem obvious that LKDTM actually does this.  It seems more focused on
> creating crashdumps than testing that (in this instance) write
> protection works - and it seems to be a destructive test.

The WRITE_RO and EXEC_RODATA tests perform similar tests, but yes, it
is intentionally destructive: it is exercising the entire protection
and kernel oops, etc.

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>>> index bdd283b..741e2e8 100644
>>> --- a/arch/arm/include/asm/cacheflush.h
>>> +++ b/arch/arm/include/asm/cacheflush.h
>>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>>  static inline void set_kernel_text_ro(void) { }
>>>  #endif
>>>  
>>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>>> +extern const int rodata_test_data;
>>> +int rodata_test(void);
>>> +#else
>>> +static inline int rodata_test(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
> 
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
> 

cacheflush.h seems to be where all the set_memory_* functions have
ended up. I was just looking at cleaning that up unless someone
beats me to it.

Thanks,
Laura

Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 03:38:52PM -0800, Laura Abbott wrote:
> On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> > I don't see why this needs to be in cacheflush.h - it doesn't seem to
> > have anything to do with cache flushing, and placing it in here means
> > that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> > the entire kernel gets rebuilt.  Please put it in a separate header
> > file.
> 
> cacheflush.h seems to be where all the set_memory_* functions have
> ended up. I was just looking at cleaning that up unless someone
> beats me to it.

+1

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.