* [PATCH] ARM: mm: add testcases for RODATA @ 2017-01-18 13:53 Jinbum Park 2017-01-18 14:38 ` Mark Rutland 2017-01-18 19:20 ` Laura Abbott 0 siblings, 2 replies; 8+ messages in thread From: Jinbum Park @ 2017-01-18 13:53 UTC (permalink / raw) To: linux Cc: will.deacon, mingo, andy.gross, keescook, vladimir.murzin, f.fainelli, pawel.moll, jonathan.austin, mark.rutland, ard.biesheuvel, labbott, arjan, paul.gortmaker, linux-arm-kernel, linux-kernel, kernel-hardening, kernel-janitors 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 13:53 [PATCH] ARM: mm: add testcases for RODATA Jinbum Park @ 2017-01-18 14:38 ` Mark Rutland 2017-01-18 19:20 ` Laura Abbott 1 sibling, 0 replies; 8+ messages in thread From: Mark Rutland @ 2017-01-18 14:38 UTC (permalink / raw) To: Jinbum Park Cc: linux, will.deacon, mingo, andy.gross, keescook, vladimir.murzin, f.fainelli, pawel.moll, jonathan.austin, ard.biesheuvel, labbott, arjan, paul.gortmaker, linux-arm-kernel, linux-kernel, kernel-hardening, kernel-janitors 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 13:53 [PATCH] ARM: mm: add testcases for RODATA Jinbum Park 2017-01-18 14:38 ` Mark Rutland @ 2017-01-18 19:20 ` Laura Abbott 2017-01-18 21:20 ` Kees Cook 2017-01-18 22:36 ` Russell King - ARM Linux 1 sibling, 2 replies; 8+ messages in thread From: Laura Abbott @ 2017-01-18 19:20 UTC (permalink / raw) To: Jinbum Park, linux Cc: will.deacon, mingo, andy.gross, keescook, vladimir.murzin, f.fainelli, pawel.moll, jonathan.austin, mark.rutland, ard.biesheuvel, arjan, paul.gortmaker, linux-arm-kernel, linux-kernel, kernel-hardening, kernel-janitors 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 19:20 ` Laura Abbott @ 2017-01-18 21:20 ` Kees Cook 2017-01-18 22:36 ` Russell King - ARM Linux 1 sibling, 0 replies; 8+ messages in thread From: Kees Cook @ 2017-01-18 21:20 UTC (permalink / raw) To: Laura Abbott Cc: Jinbum Park, Russell King, Will Deacon, Ingo Molnar, andy.gross, Vladimir Murzin, f.fainelli, Pawel Moll, Jonathan Austin, Mark Rutland, Ard Biesheuvel, Arjan van de Ven, Paul Gortmaker, linux-arm-kernel, LKML, kernel-hardening, kernel-janitors 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 19:20 ` Laura Abbott 2017-01-18 21:20 ` Kees Cook @ 2017-01-18 22:36 ` Russell King - ARM Linux 2017-01-18 23:35 ` Kees Cook 2017-01-18 23:38 ` [kernel-hardening] " Laura Abbott 1 sibling, 2 replies; 8+ messages in thread From: Russell King - ARM Linux @ 2017-01-18 22:36 UTC (permalink / raw) To: Laura Abbott Cc: Jinbum Park, mark.rutland, f.fainelli, keescook, pawel.moll, ard.biesheuvel, kernel-janitors, kernel-hardening, will.deacon, linux-kernel, paul.gortmaker, vladimir.murzin, andy.gross, arjan, mingo, linux-arm-kernel, jonathan.austin 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 22:36 ` Russell King - ARM Linux @ 2017-01-18 23:35 ` Kees Cook 2017-01-18 23:38 ` [kernel-hardening] " Laura Abbott 1 sibling, 0 replies; 8+ messages in thread From: Kees Cook @ 2017-01-18 23:35 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Laura Abbott, Jinbum Park, Mark Rutland, Florian Fainelli, Pawel Moll, Ard Biesheuvel, kernel-janitors, kernel-hardening, Will Deacon, LKML, Paul Gortmaker, Vladimir Murzin, Andy Gross, Arjan van de Ven, Ingo Molnar, linux-arm-kernel, Jonathan Austin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 22:36 ` Russell King - ARM Linux 2017-01-18 23:35 ` Kees Cook @ 2017-01-18 23:38 ` Laura Abbott 2017-01-18 23:45 ` Russell King - ARM Linux 1 sibling, 1 reply; 8+ messages in thread From: Laura Abbott @ 2017-01-18 23:38 UTC (permalink / raw) To: kernel-hardening Cc: Jinbum Park, mark.rutland, f.fainelli, keescook, pawel.moll, ard.biesheuvel, kernel-janitors, will.deacon, linux-kernel, paul.gortmaker, vladimir.murzin, andy.gross, arjan, mingo, linux-arm-kernel, jonathan.austin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA 2017-01-18 23:38 ` [kernel-hardening] " Laura Abbott @ 2017-01-18 23:45 ` Russell King - ARM Linux 0 siblings, 0 replies; 8+ messages in thread From: Russell King - ARM Linux @ 2017-01-18 23:45 UTC (permalink / raw) To: Laura Abbott Cc: kernel-hardening, mark.rutland, f.fainelli, keescook, pawel.moll, ard.biesheuvel, will.deacon, kernel-janitors, linux-kernel, Jinbum Park, vladimir.murzin, linux-arm-kernel, andy.gross, arjan, mingo, paul.gortmaker, jonathan.austin 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-19 0:06 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-18 13:53 [PATCH] ARM: mm: add testcases for RODATA Jinbum Park 2017-01-18 14:38 ` Mark Rutland 2017-01-18 19:20 ` Laura Abbott 2017-01-18 21:20 ` Kees Cook 2017-01-18 22:36 ` Russell King - ARM Linux 2017-01-18 23:35 ` Kees Cook 2017-01-18 23:38 ` [kernel-hardening] " Laura Abbott 2017-01-18 23:45 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).