[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 18 Jan 2017 15:35:37 -0800
From: Kees Cook <keescook@...omium.org>
To: Russell King - ARM Linux <linux@...linux.org.uk>
Cc: Laura Abbott <labbott@...hat.com>, Jinbum Park <jinb.park7@...il.com>,
Mark Rutland <mark.rutland@....com>, Florian Fainelli <f.fainelli@...il.com>,
Pawel Moll <pawel.moll@....com>, Ard Biesheuvel <ard.biesheuvel@...aro.org>,
kernel-janitors@...r.kernel.org,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, Will Deacon <will.deacon@....com>,
LKML <linux-kernel@...r.kernel.org>,
Paul Gortmaker <paul.gortmaker@...driver.com>, Vladimir Murzin <vladimir.murzin@....com>,
Andy Gross <andy.gross@...aro.org>, Arjan van de Ven <arjan@...ux.intel.com>,
Ingo Molnar <mingo@...nel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
Jonathan Austin <jonathan.austin@....com>
Subject: Re: [PATCH] ARM: mm: add testcases for RODATA
On Wed, Jan 18, 2017 at 2:36 PM, Russell King - ARM Linux
<linux@...linux.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@...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?
>
> 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
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.
