[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e5433e24-7a3d-25a4-26a3-9d02644cc3f4@redhat.com>
Date: Wed, 18 Jan 2017 11:20:54 -0800
From: Laura Abbott <labbott@...hat.com>
To: Jinbum Park <jinb.park7@...il.com>, linux@...linux.org.uk
Cc: will.deacon@....com, mingo@...nel.org, andy.gross@...aro.org,
keescook@...omium.org, vladimir.murzin@....com, f.fainelli@...il.com,
pawel.moll@....com, jonathan.austin@....com, mark.rutland@....com,
ard.biesheuvel@...aro.org, arjan@...ux.intel.com,
paul.gortmaker@...driver.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com,
kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ARM: mm: add testcases for RODATA
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@...il.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@...il.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
