Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335874 > unrolled thread

[PATCH] lkdtm: add test for executing .rodata

Back to article view | Back to linux.kernel

Contents

  [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-16 22:50 +0100
    Re: [PATCH] lkdtm: add test for executing .rodata Laura Abbott <labbott@redhat.com> - 2016-02-17 02:10 +0100
      Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-17 21:30 +0100
        Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-17 22:10 +0100
        Re: [PATCH] lkdtm: add test for executing .rodata "PaX Team" <pageexec@freemail.hu> - 2016-02-18 11:40 +0100
          Re: [PATCH] lkdtm: add test for executing .rodata Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-18 12:40 +0100
            Re: [PATCH] lkdtm: add test for executing .rodata Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-18 13:00 +0100
            Re: [PATCH] lkdtm: add test for executing .rodata Arnd Bergmann <arnd@arndb.de> - 2016-02-18 13:10 +0100
              Re: [PATCH] lkdtm: add test for executing .rodata Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-18 13:50 +0100
                Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-18 21:10 +0100
            Re: [PATCH] lkdtm: add test for executing .rodata "PaX Team" <pageexec@freemail.hu> - 2016-02-18 22:30 +0100
              Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-22 21:50 +0100
                Re: [PATCH] lkdtm: add test for executing .rodata "PaX Team" <pageexec@freemail.hu> - 2016-02-23 00:30 +0100
                  Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-23 22:00 +0100
                    Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-23 23:10 +0100
                    Re: [PATCH] lkdtm: add test for executing .rodata "PaX Team" <pageexec@freemail.hu> - 2016-02-23 23:10 +0100
                Re: [kernel-hardening] Re: [PATCH] lkdtm: add test for executing  .rodata David Brown <david.brown@linaro.org> - 2016-02-23 21:40 +0100
                  Re: [kernel-hardening] Re: [PATCH] lkdtm: add test for executing .rodata Kees Cook <keescook@chromium.org> - 2016-02-23 22:00 +0100
    Re: [PATCH] lkdtm: add test for executing .rodata Arnd Bergmann <arnd@arndb.de> - 2016-02-17 22:50 +0100
    Re: [PATCH] lkdtm: add test for executing .rodata Arnd Bergmann <arnd@arndb.de> - 2016-02-17 22:50 +0100

#1335874 — [PATCH] lkdtm: add test for executing .rodata

Make sure that the read-only data section isn't executable.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 11fdadc68e53..9835fcc0506e 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -100,6 +100,7 @@ enum ctype {
 	CT_EXEC_STACK,
 	CT_EXEC_KMALLOC,
 	CT_EXEC_VMALLOC,
+	CT_EXEC_RODATA,
 	CT_EXEC_USERSPACE,
 	CT_ACCESS_USERSPACE,
 	CT_WRITE_RO,
@@ -137,6 +138,7 @@ static char* cp_type[] = {
 	"EXEC_STACK",
 	"EXEC_KMALLOC",
 	"EXEC_VMALLOC",
+	"EXEC_RODATA",
 	"EXEC_USERSPACE",
 	"ACCESS_USERSPACE",
 	"WRITE_RO",
@@ -315,6 +317,12 @@ static int recursive_loop(int remaining)
 		return recursive_loop(remaining - 1);
 }
 
+static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
+do_nothing_rodata(void)
+{
+	return;
+}
+
 static void do_nothing(void)
 {
 	return;
@@ -335,15 +343,18 @@ static noinline void corrupt_stack(void)
 	memset((void *)data, 0, 64);
 }
 
-static void execute_location(void *dst)
+static void execute_location(void *dst, bool write)
 {
 	void (*func)(void) = dst;
 
 	pr_info("attempting ok execution at %p\n", do_nothing);
 	do_nothing();
 
-	memcpy(dst, do_nothing, EXEC_SIZE);
-	flush_icache_range((unsigned long)dst, (unsigned long)dst + EXEC_SIZE);
+	if (write) {
+		memcpy(dst, do_nothing, EXEC_SIZE);
+		flush_icache_range((unsigned long)dst,
+				   (unsigned long)dst + EXEC_SIZE);
+	}
 	pr_info("attempting bad execution at %p\n", func);
 	func();
 }
@@ -438,25 +449,28 @@ static void lkdtm_do_action(enum ctype which)
 		schedule();
 		break;
 	case CT_EXEC_DATA:
-		execute_location(data_area);
+		execute_location(data_area, true);
 		break;
 	case CT_EXEC_STACK: {
 		u8 stack_area[EXEC_SIZE];
-		execute_location(stack_area);
+		execute_location(stack_area, true);
 		break;
 	}
 	case CT_EXEC_KMALLOC: {
 		u32 *kmalloc_area = kmalloc(EXEC_SIZE, GFP_KERNEL);
-		execute_location(kmalloc_area);
+		execute_location(kmalloc_area, true);
 		kfree(kmalloc_area);
 		break;
 	}
 	case CT_EXEC_VMALLOC: {
 		u32 *vmalloc_area = vmalloc(EXEC_SIZE);
-		execute_location(vmalloc_area);
+		execute_location(vmalloc_area, true);
 		vfree(vmalloc_area);
 		break;
 	}
+	case CT_EXEC_RODATA:
+		execute_location(do_nothing_rodata, false);
+		break;
 	case CT_EXEC_USERSPACE: {
 		unsigned long user_addr;
 
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [next] | [standalone]

#1335973

On 02/16/2016 01:49 PM, Kees Cook wrote:
> Make sure that the read-only data section isn't executable.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   drivers/misc/lkdtm.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
> index 11fdadc68e53..9835fcc0506e 100644
> --- a/drivers/misc/lkdtm.c
> +++ b/drivers/misc/lkdtm.c
> @@ -100,6 +100,7 @@ enum ctype {
>   	CT_EXEC_STACK,
>   	CT_EXEC_KMALLOC,
>   	CT_EXEC_VMALLOC,
> +	CT_EXEC_RODATA,
>   	CT_EXEC_USERSPACE,
>   	CT_ACCESS_USERSPACE,
>   	CT_WRITE_RO,
> @@ -137,6 +138,7 @@ static char* cp_type[] = {
>   	"EXEC_STACK",
>   	"EXEC_KMALLOC",
>   	"EXEC_VMALLOC",
> +	"EXEC_RODATA",
>   	"EXEC_USERSPACE",
>   	"ACCESS_USERSPACE",
>   	"WRITE_RO",
> @@ -315,6 +317,12 @@ static int recursive_loop(int remaining)
>   		return recursive_loop(remaining - 1);
>   }
>
> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
> +do_nothing_rodata(void)
> +{
> +	return;
> +}
> +

>

This doesn't cross compile for me on arm64 with two different toolchains

CC drivers/misc/lkdtm.o
/tmp/ccHzIWIx.s: Assembler messages:
/tmp/ccHzIWIx.s:21: Error: junk at end of line, first unrecognized character is `#'
/tmp/ccHzIWIx.s: Error: unaligned opcodes detected in executable segment
scripts/Makefile.build:258: recipe for target 'drivers/misc/lkdtm.o' failed
make[2]: *** [drivers/misc/lkdtm.o] Error 1
scripts/Makefile.build:407: recipe for target 'drivers/misc' failed
make[1]: *** [drivers/misc] Error 2
Makefile:950: recipe for target 'drivers' failed
make: *** [drivers] Error 2

I don't know the assembler well enough to give any insight.

Thanks,
Laura

>   static void do_nothing(void)
>   {
>   	return;
> @@ -335,15 +343,18 @@ static noinline void corrupt_stack(void)
>   	memset((void *)data, 0, 64);
>   }
>
> -static void execute_location(void *dst)
> +static void execute_location(void *dst, bool write)
>   {
>   	void (*func)(void) = dst;
>
>   	pr_info("attempting ok execution at %p\n", do_nothing);
>   	do_nothing();
>
> -	memcpy(dst, do_nothing, EXEC_SIZE);
> -	flush_icache_range((unsigned long)dst, (unsigned long)dst + EXEC_SIZE);
> +	if (write) {
> +		memcpy(dst, do_nothing, EXEC_SIZE);
> +		flush_icache_range((unsigned long)dst,
> +				   (unsigned long)dst + EXEC_SIZE);
> +	}
>   	pr_info("attempting bad execution at %p\n", func);
>   	func();
>   }
> @@ -438,25 +449,28 @@ static void lkdtm_do_action(enum ctype which)
>   		schedule();
>   		break;
>   	case CT_EXEC_DATA:
> -		execute_location(data_area);
> +		execute_location(data_area, true);
>   		break;
>   	case CT_EXEC_STACK: {
>   		u8 stack_area[EXEC_SIZE];
> -		execute_location(stack_area);
> +		execute_location(stack_area, true);
>   		break;
>   	}
>   	case CT_EXEC_KMALLOC: {
>   		u32 *kmalloc_area = kmalloc(EXEC_SIZE, GFP_KERNEL);
> -		execute_location(kmalloc_area);
> +		execute_location(kmalloc_area, true);
>   		kfree(kmalloc_area);
>   		break;
>   	}
>   	case CT_EXEC_VMALLOC: {
>   		u32 *vmalloc_area = vmalloc(EXEC_SIZE);
> -		execute_location(vmalloc_area);
> +		execute_location(vmalloc_area, true);
>   		vfree(vmalloc_area);
>   		break;
>   	}
> +	case CT_EXEC_RODATA:
> +		execute_location(do_nothing_rodata, false);
> +		break;
>   	case CT_EXEC_USERSPACE: {
>   		unsigned long user_addr;
>
>

[toc] | [prev] | [next] | [standalone]

#1336732

On Tue, Feb 16, 2016 at 5:06 PM, Laura Abbott <labbott@redhat.com> wrote:
>
>
> On 02/16/2016 01:49 PM, Kees Cook wrote:
>>
>> Make sure that the read-only data section isn't executable.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>   drivers/misc/lkdtm.c | 28 +++++++++++++++++++++-------
>>   1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
>> index 11fdadc68e53..9835fcc0506e 100644
>> --- a/drivers/misc/lkdtm.c
>> +++ b/drivers/misc/lkdtm.c
>> @@ -100,6 +100,7 @@ enum ctype {
>>         CT_EXEC_STACK,
>>         CT_EXEC_KMALLOC,
>>         CT_EXEC_VMALLOC,
>> +       CT_EXEC_RODATA,
>>         CT_EXEC_USERSPACE,
>>         CT_ACCESS_USERSPACE,
>>         CT_WRITE_RO,
>> @@ -137,6 +138,7 @@ static char* cp_type[] = {
>>         "EXEC_STACK",
>>         "EXEC_KMALLOC",
>>         "EXEC_VMALLOC",
>> +       "EXEC_RODATA",
>>         "EXEC_USERSPACE",
>>         "ACCESS_USERSPACE",
>>         "WRITE_RO",
>> @@ -315,6 +317,12 @@ static int recursive_loop(int remaining)
>>                 return recursive_loop(remaining - 1);
>>   }
>>
>> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
>> +do_nothing_rodata(void)
>> +{
>> +       return;
>> +}
>> +
>
>
>>
>
> This doesn't cross compile for me on arm64 with two different toolchains
>
> CC drivers/misc/lkdtm.o
> /tmp/ccHzIWIx.s: Assembler messages:
> /tmp/ccHzIWIx.s:21: Error: junk at end of line, first unrecognized character
> is `#'
> /tmp/ccHzIWIx.s: Error: unaligned opcodes detected in executable segment
> scripts/Makefile.build:258: recipe for target 'drivers/misc/lkdtm.o' failed
> make[2]: *** [drivers/misc/lkdtm.o] Error 1
> scripts/Makefile.build:407: recipe for target 'drivers/misc' failed
> make[1]: *** [drivers/misc] Error 2
> Makefile:950: recipe for target 'drivers' failed
> make: *** [drivers] Error 2
>
> I don't know the assembler well enough to give any insight.

Hm, bummer. I was trying to get fancy with the function forced into
.rodata by trying to force the bits. Looks like "#" is not seen as a
comment character by the toolchain you're using.

Anyone else successfully done tricks like this?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1336762

On Wed, Feb 17, 2016 at 12:29 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Feb 16, 2016 at 5:06 PM, Laura Abbott <labbott@redhat.com> wrote:
>>
>>
>> On 02/16/2016 01:49 PM, Kees Cook wrote:
>>>
>>> Make sure that the read-only data section isn't executable.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>>   drivers/misc/lkdtm.c | 28 +++++++++++++++++++++-------
>>>   1 file changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
>>> index 11fdadc68e53..9835fcc0506e 100644
>>> --- a/drivers/misc/lkdtm.c
>>> +++ b/drivers/misc/lkdtm.c
>>> @@ -100,6 +100,7 @@ enum ctype {
>>>         CT_EXEC_STACK,
>>>         CT_EXEC_KMALLOC,
>>>         CT_EXEC_VMALLOC,
>>> +       CT_EXEC_RODATA,
>>>         CT_EXEC_USERSPACE,
>>>         CT_ACCESS_USERSPACE,
>>>         CT_WRITE_RO,
>>> @@ -137,6 +138,7 @@ static char* cp_type[] = {
>>>         "EXEC_STACK",
>>>         "EXEC_KMALLOC",
>>>         "EXEC_VMALLOC",
>>> +       "EXEC_RODATA",
>>>         "EXEC_USERSPACE",
>>>         "ACCESS_USERSPACE",
>>>         "WRITE_RO",
>>> @@ -315,6 +317,12 @@ static int recursive_loop(int remaining)
>>>                 return recursive_loop(remaining - 1);
>>>   }
>>>
>>> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
>>> +do_nothing_rodata(void)
>>> +{
>>> +       return;
>>> +}
>>> +
>>
>>
>>>
>>
>> This doesn't cross compile for me on arm64 with two different toolchains
>>
>> CC drivers/misc/lkdtm.o
>> /tmp/ccHzIWIx.s: Assembler messages:
>> /tmp/ccHzIWIx.s:21: Error: junk at end of line, first unrecognized character
>> is `#'
>> /tmp/ccHzIWIx.s: Error: unaligned opcodes detected in executable segment
>> scripts/Makefile.build:258: recipe for target 'drivers/misc/lkdtm.o' failed
>> make[2]: *** [drivers/misc/lkdtm.o] Error 1
>> scripts/Makefile.build:407: recipe for target 'drivers/misc' failed
>> make[1]: *** [drivers/misc] Error 2
>> Makefile:950: recipe for target 'drivers' failed
>> make: *** [drivers] Error 2
>>
>> I don't know the assembler well enough to give any insight.
>
> Hm, bummer. I was trying to get fancy with the function forced into
> .rodata by trying to force the bits. Looks like "#" is not seen as a
> comment character by the toolchain you're using.

/me cries: the comment character is arch-specific (# on x86, @ on arm).

Looks like "//" works, but only at the start of a new line, and ";" is
seen as a new line start, so ";//" should work everywhere... I'll send
a v2, build and runtested on x86 and arm, and we'll see if the
buildbot kicks out any other cross compile failures...

-Kees

> Anyone else successfully done tricks like this?
>
> -Kees
>
> --
> Kees Cook
> Chrome OS & Brillo Security



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1337253

On 17 Feb 2016 at 12:29, Kees Cook wrote:

> >> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
> >> +do_nothing_rodata(void)
> >> +{
> >> +       return;
> >> +}
> >> +
> >
> >
> >>
> >
> > This doesn't cross compile for me on arm64 with two different toolchains
> >
> > CC drivers/misc/lkdtm.o
> > /tmp/ccHzIWIx.s: Assembler messages:
> > /tmp/ccHzIWIx.s:21: Error: junk at end of line, first unrecognized character
> > is `#'
> > /tmp/ccHzIWIx.s: Error: unaligned opcodes detected in executable segment
> > scripts/Makefile.build:258: recipe for target 'drivers/misc/lkdtm.o' failed
> > make[2]: *** [drivers/misc/lkdtm.o] Error 1
> > scripts/Makefile.build:407: recipe for target 'drivers/misc' failed
> > make[1]: *** [drivers/misc] Error 2
> > Makefile:950: recipe for target 'drivers' failed
> > make: *** [drivers] Error 2
> >
> > I don't know the assembler well enough to give any insight.
> 
> Hm, bummer. I was trying to get fancy with the function forced into
> .rodata by trying to force the bits. Looks like "#" is not seen as a
> comment character by the toolchain you're using.
> 
> Anyone else successfully done tricks like this?

wouldn't it be a better and more generic/reusable approach to

#define __ro_text __attribute__((__section__(".rodata.text")))

and move this function there by the linker script similar to how it's done
for other code that goes into special sections?

[toc] | [prev] | [next] | [standalone]

#1337290

On 18 February 2016 at 11:32, PaX Team <pageexec@freemail.hu> wrote:
> On 17 Feb 2016 at 12:29, Kees Cook wrote:
>
>> >> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
>> >> +do_nothing_rodata(void)
>> >> +{
>> >> +       return;
>> >> +}
>> >> +
>> >
>> >
>> >>
>> >
>> > This doesn't cross compile for me on arm64 with two different toolchains
>> >
>> > CC drivers/misc/lkdtm.o
>> > /tmp/ccHzIWIx.s: Assembler messages:
>> > /tmp/ccHzIWIx.s:21: Error: junk at end of line, first unrecognized character
>> > is `#'
>> > /tmp/ccHzIWIx.s: Error: unaligned opcodes detected in executable segment
>> > scripts/Makefile.build:258: recipe for target 'drivers/misc/lkdtm.o' failed
>> > make[2]: *** [drivers/misc/lkdtm.o] Error 1
>> > scripts/Makefile.build:407: recipe for target 'drivers/misc' failed
>> > make[1]: *** [drivers/misc] Error 2
>> > Makefile:950: recipe for target 'drivers' failed
>> > make: *** [drivers] Error 2
>> >
>> > I don't know the assembler well enough to give any insight.
>>
>> Hm, bummer. I was trying to get fancy with the function forced into
>> .rodata by trying to force the bits. Looks like "#" is not seen as a
>> comment character by the toolchain you're using.
>>
>> Anyone else successfully done tricks like this?
>
> wouldn't it be a better and more generic/reusable approach to
>
> #define __ro_text __attribute__((__section__(".rodata.text")))
>
> and move this function there by the linker script similar to how it's done
> for other code that goes into special sections?
>

We have __section() as an alias for __attribute__((__section__())), so
we could use that instead.

However, that does not fix the issue Kees is trying to solve, where a
.rodata section is emitted with the "x" bit set, which causes the
linker to complain:

/tmp/cc50ffWw.s: Assembler messages:
/tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
.rodata.text

I wonder if we could get away with doing something like

AFLAGS_lkdtm.o += -Wa,-W

here? This just hides the warnings, but may result in the .rodata
section in the vmlinux file to have X permissions as well. I don't
think anyone uses an ELF loader to load their kernel, but who knows
...

That only matters when lkdtm is a module, btw

[toc] | [prev] | [next] | [standalone]

#1337298

On 18 February 2016 at 12:34, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 18 February 2016 at 11:32, PaX Team <pageexec@freemail.hu> wrote:
>> On 17 Feb 2016 at 12:29, Kees Cook wrote:
>>
>>> >> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
>>> >> +do_nothing_rodata(void)
>>> >> +{
>>> >> +       return;
>>> >> +}
>>> >> +
>>> >
>>> >
>>> >>
>>> >
>>> > This doesn't cross compile for me on arm64 with two different toolchains
>>> >
>>> > CC drivers/misc/lkdtm.o
>>> > /tmp/ccHzIWIx.s: Assembler messages:
>>> > /tmp/ccHzIWIx.s:21: Error: junk at end of line, first unrecognized character
>>> > is `#'
>>> > /tmp/ccHzIWIx.s: Error: unaligned opcodes detected in executable segment
>>> > scripts/Makefile.build:258: recipe for target 'drivers/misc/lkdtm.o' failed
>>> > make[2]: *** [drivers/misc/lkdtm.o] Error 1
>>> > scripts/Makefile.build:407: recipe for target 'drivers/misc' failed
>>> > make[1]: *** [drivers/misc] Error 2
>>> > Makefile:950: recipe for target 'drivers' failed
>>> > make: *** [drivers] Error 2
>>> >
>>> > I don't know the assembler well enough to give any insight.
>>>
>>> Hm, bummer. I was trying to get fancy with the function forced into
>>> .rodata by trying to force the bits. Looks like "#" is not seen as a
>>> comment character by the toolchain you're using.
>>>
>>> Anyone else successfully done tricks like this?
>>
>> wouldn't it be a better and more generic/reusable approach to
>>
>> #define __ro_text __attribute__((__section__(".rodata.text")))
>>
>> and move this function there by the linker script similar to how it's done
>> for other code that goes into special sections?
>>
>
> We have __section() as an alias for __attribute__((__section__())), so
> we could use that instead.
>
> However, that does not fix the issue Kees is trying to solve, where a
> .rodata section is emitted with the "x" bit set, which causes the
> linker to complain:
>
> /tmp/cc50ffWw.s: Assembler messages:
> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
> .rodata.text
>
> I wonder if we could get away with doing something like
>
> AFLAGS_lkdtm.o += -Wa,-W
>
> here? This just hides the warnings, but may result in the .rodata
> section in the vmlinux file to have X permissions as well. I don't
> think anyone uses an ELF loader to load their kernel, but who knows
> ...
>
> That only matters when lkdtm is a module, btw

... is NOT a module, obviously

[toc] | [prev] | [next] | [standalone]

#1337299

On Thursday 18 February 2016 12:34:50 Ard Biesheuvel wrote:
> 
> We have __section() as an alias for __attribute__((__section__())), so
> we could use that instead.
> 
> However, that does not fix the issue Kees is trying to solve, where a
> .rodata section is emitted with the "x" bit set, which causes the
> linker to complain:
> 
> /tmp/cc50ffWw.s: Assembler messages:
> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
> .rodata.text
> 
> I wonder if we could get away with doing something like
> 
> AFLAGS_lkdtm.o += -Wa,-W
> 
> here? This just hides the warnings, but may result in the .rodata
> section in the vmlinux file to have X permissions as well. I don't
> think anyone uses an ELF loader to load their kernel, but who knows
> ...

Don't we also get a warning when we link objects with conflicting
section attributes?

Maybe a solution would be to define a separate section for this one
function, and then use a linker script to move it into .rodata?
Or maybe "objcopy --set-section-flags  --rename-section"?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1337322

On 18 February 2016 at 13:07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 18 February 2016 12:34:50 Ard Biesheuvel wrote:
>>
>> We have __section() as an alias for __attribute__((__section__())), so
>> we could use that instead.
>>
>> However, that does not fix the issue Kees is trying to solve, where a
>> .rodata section is emitted with the "x" bit set, which causes the
>> linker to complain:
>>
>> /tmp/cc50ffWw.s: Assembler messages:
>> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
>> .rodata.text
>>
>> I wonder if we could get away with doing something like
>>
>> AFLAGS_lkdtm.o += -Wa,-W
>>
>> here? This just hides the warnings, but may result in the .rodata
>> section in the vmlinux file to have X permissions as well. I don't
>> think anyone uses an ELF loader to load their kernel, but who knows
>> ...
>
> Don't we also get a warning when we link objects with conflicting
> section attributes?
>

I didn't see one

> Maybe a solution would be to define a separate section for this one
> function, and then use a linker script to move it into .rodata?
> Or maybe "objcopy --set-section-flags  --rename-section"?
>

I think objcopy is the easiest.

[toc] | [prev] | [next] | [standalone]

#1337664

On Thu, Feb 18, 2016 at 4:46 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 18 February 2016 at 13:07, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 18 February 2016 12:34:50 Ard Biesheuvel wrote:
>>>
>>> We have __section() as an alias for __attribute__((__section__())), so
>>> we could use that instead.
>>>
>>> However, that does not fix the issue Kees is trying to solve, where a
>>> .rodata section is emitted with the "x" bit set, which causes the
>>> linker to complain:
>>>
>>> /tmp/cc50ffWw.s: Assembler messages:
>>> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
>>> .rodata.text
>>>
>>> I wonder if we could get away with doing something like
>>>
>>> AFLAGS_lkdtm.o += -Wa,-W
>>>
>>> here? This just hides the warnings, but may result in the .rodata
>>> section in the vmlinux file to have X permissions as well. I don't
>>> think anyone uses an ELF loader to load their kernel, but who knows
>>> ...
>>
>> Don't we also get a warning when we link objects with conflicting
>> section attributes?
>>
>
> I didn't see one
>
>> Maybe a solution would be to define a separate section for this one
>> function, and then use a linker script to move it into .rodata?
>> Or maybe "objcopy --set-section-flags  --rename-section"?
>>
>
> I think objcopy is the easiest.

I came to the same conclusion while thinking about it last night. I'm
having a terrible time implementing it in kbuild, though. The
"objcopy" function expects to rename the files, and I can't find a way
to just add it on without breaking the module build.

The flags I'm using that seem to actually do what's needed are (when
using the section name ".text.rodata"):

OBJCOPYFLAGS_lkdtm.o := --set-section-flags .text.rodata=alloc,readonly \
                        --rename-section .text.rodata=.rodata

So the diff against my existing patch looks like this, and works for
CONFIG_LKDTM=y (sorry for whitespace damage...):

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 537d7f3b78da..55ce014e6b62 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_BMP085_I2C)      += bmp085-i2c.o
 obj-$(CONFIG_BMP085_SPI)       += bmp085-spi.o
 obj-$(CONFIG_DUMMY_IRQ)                += dummy-irq.o
 obj-$(CONFIG_ICS932S401)       += ics932s401.o
-obj-$(CONFIG_LKDTM)            += lkdtm.o
+obj-$(CONFIG_LKDTM)            += lkdtm_objcopy.o
 obj-$(CONFIG_TIFM_CORE)        += tifm_core.o
 obj-$(CONFIG_TIFM_7XX1)        += tifm_7xx1.o
 obj-$(CONFIG_PHANTOM)          += phantom.o
@@ -56,3 +56,8 @@ obj-$(CONFIG_GENWQE)          += genwqe/
 obj-$(CONFIG_ECHO)             += echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)  += vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)         += cxl/
+
+OBJCOPYFLAGS_lkdtm_objcopy.o := --set-section-flags
.text.rodata=alloc,readonly \
+                       --rename-section .text.rodata=.rodata
+$(obj)/lkdtm_objcopy.o: $(obj)/lkdtm.o FORCE
+       $(call if_changed,objcopy)
diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index b15d08ff71a9..5868e5125fbe 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -317,8 +317,7 @@ static int recursive_loop(int remaining)
                return recursive_loop(remaining - 1);
 }

-static void __attribute__((__section__(".rodata,\"a\",\%progbits;//")))
-do_nothing_rodata(void)
+static void __section(.text.rodata) do_nothing_rodata(void)
 {
        return;
 }


But fails in ways I don't understand for CONFIG_LKDTM=m:

./scripts/Makefile.build:264: warning: overriding commands for target `drivers/m
isc/lkdtm_objcopy.o'
drivers/misc/Makefile:63: warning: ignoring old commands for target `drivers/mis
c/lkdtm_objcopy.o'
make[2]: *** No rule to make target `drivers/misc/lkdtm_objcopy.c', needed by `d
rivers/misc/lkdtm_objcopy.o'.  Stop.
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers/misc] Error 2
make[1]: *** Waiting for unfinished jobs....

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1337743

On 18 Feb 2016 at 12:34, Ard Biesheuvel wrote:

> However, that does not fix the issue Kees is trying to solve, where a
> .rodata section is emitted with the "x" bit set, which causes the
> linker to complain:
> 
> /tmp/cc50ffWw.s: Assembler messages:
> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
> .rodata.text

in that case why not use a top-level asm statement to set the section
and its attributes (and compile the file with fno-toplevel-reorder)?

[toc] | [prev] | [next] | [standalone]

#1339899

On Thu, Feb 18, 2016 at 1:27 PM, PaX Team <pageexec@freemail.hu> wrote:
> On 18 Feb 2016 at 12:34, Ard Biesheuvel wrote:
>
>> However, that does not fix the issue Kees is trying to solve, where a
>> .rodata section is emitted with the "x" bit set, which causes the
>> linker to complain:
>>
>> /tmp/cc50ffWw.s: Assembler messages:
>> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
>> .rodata.text
>
> in that case why not use a top-level asm statement to set the section
> and its attributes (and compile the file with fno-toplevel-reorder)?

GCC really wants to declare the section. :(

asm(".pushsection .rodata");
static void do_nothing_rodata(void)
{
        return;
}
asm(".popsection");

With -fno-toplevel-reorder, this produces:

#APP
        .pushsection .rodata
#NO_APP
        .section        .text.unlikely
.LCOLDB42:
        .text
.LHOTB42:
        .p2align 4,,15
        .type   do_nothing_rodata, @function
do_nothing_rodata:
.LFB2756:
        .loc 1 323 0
        .cfi_startproc
        pushq   %rbp
...

So I either need to define "ret" for every architecture, define the
linker comment character for every architecture, or do some generated
file. I'll try the latter next...

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1340079

On 22 Feb 2016 at 12:46, Kees Cook wrote:

> GCC really wants to declare the section. :(

hmm, i see, so how about going about it another way. instead of trying
to do this at compile/link time, do it an load/runtime. one way of doing
it would be to preserve a page in .rodata then map in a code page underneath
that holds your empty function (which you can generate from C). it'd be
somewhat similar to how the vsyscall page on amd64 is mapped (or used to
be mapped) from the kernel image into its userland visible place.

[toc] | [prev] | [next] | [standalone]

#1341028

On Mon, Feb 22, 2016 at 3:21 PM, PaX Team <pageexec@freemail.hu> wrote:
> On 22 Feb 2016 at 12:46, Kees Cook wrote:
>
>> GCC really wants to declare the section. :(
>
> hmm, i see, so how about going about it another way. instead of trying
> to do this at compile/link time, do it an load/runtime. one way of doing
> it would be to preserve a page in .rodata then map in a code page underneath
> that holds your empty function (which you can generate from C). it'd be
> somewhat similar to how the vsyscall page on amd64 is mapped (or used to
> be mapped) from the kernel image into its userland visible place.

I prefer using all the "regular" mechanisms so that I really know I'm
exercising the actual case I want to be testing. (i.e. I don't want to
bypass the linker.)

If only there were some way to filter gcc output, like with plugins. ;)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1341116

On Tue, Feb 23, 2016 at 2:00 PM, PaX Team <pageexec@freemail.hu> wrote:
> On 23 Feb 2016 at 12:53, Kees Cook wrote:
>
>> I prefer using all the "regular" mechanisms so that I really know I'm
>> exercising the actual case I want to be testing. (i.e. I don't want to
>> bypass the linker.)
>>
>> If only there were some way to filter gcc output, like with plugins. ;)
>
> plugins can set/override section flags, initify and constify both do in
> fact but i guess that's little help for your generic case that has to
> work without them as well.

I don't mind using a plugin to get the linker to see the right flags. :)

I'm hoping someone will take pity on me in the kbuild list.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1341122

On 23 Feb 2016 at 12:53, Kees Cook wrote:

> I prefer using all the "regular" mechanisms so that I really know I'm
> exercising the actual case I want to be testing. (i.e. I don't want to
> bypass the linker.)
> 
> If only there were some way to filter gcc output, like with plugins. ;)

plugins can set/override section flags, initify and constify both do in
fact but i guess that's little help for your generic case that has to
work without them as well.

[toc] | [prev] | [next] | [standalone]

#1341008 — Re: [kernel-hardening] Re: [PATCH] lkdtm: add test for executing .rodata

On Mon, Feb 22, 2016 at 12:46:28PM -0800, Kees Cook wrote:
>On Thu, Feb 18, 2016 at 1:27 PM, PaX Team <pageexec@freemail.hu> wrote:
>> On 18 Feb 2016 at 12:34, Ard Biesheuvel wrote:
>>
>>> However, that does not fix the issue Kees is trying to solve, where a
>>> .rodata section is emitted with the "x" bit set, which causes the
>>> linker to complain:
>>>
>>> /tmp/cc50ffWw.s: Assembler messages:
>>> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
>>> .rodata.text
>>
>> in that case why not use a top-level asm statement to set the section
>> and its attributes (and compile the file with fno-toplevel-reorder)?
>
>GCC really wants to declare the section. :(

Why not then just use its mechanism to set the section on the code?

static void do_nothing_rodata(void)
	__attribute__((section(".rodata")))
{
	return;
}

David

[toc] | [prev] | [next] | [standalone]

#1341024 — Re: [kernel-hardening] Re: [PATCH] lkdtm: add test for executing .rodata

On Tue, Feb 23, 2016 at 12:31 PM, David Brown <david.brown@linaro.org> wrote:
> On Mon, Feb 22, 2016 at 12:46:28PM -0800, Kees Cook wrote:
>>
>> On Thu, Feb 18, 2016 at 1:27 PM, PaX Team <pageexec@freemail.hu> wrote:
>>>
>>> On 18 Feb 2016 at 12:34, Ard Biesheuvel wrote:
>>>
>>>> However, that does not fix the issue Kees is trying to solve, where a
>>>> .rodata section is emitted with the "x" bit set, which causes the
>>>> linker to complain:
>>>>
>>>> /tmp/cc50ffWw.s: Assembler messages:
>>>> /tmp/cc50ffWw.s:2: Warning: setting incorrect section attributes for
>>>> .rodata.text
>>>
>>>
>>> in that case why not use a top-level asm statement to set the section
>>> and its attributes (and compile the file with fno-toplevel-reorder)?
>>
>>
>> GCC really wants to declare the section. :(
>
>
> Why not then just use its mechanism to set the section on the code?

It's noted in the quoted section above. :) The problem is that gcc emits:

.section ".rodata","ax",@progbits

But we need the flags to be "a" not "ax". (And I'd note that even "@"
is arch-specific. % is used when @ is a comment start character.)

objcopy can get me the "a", but it seems to require renaming lkdtm.c,
which I really don't want to do.

-Kees

>
> static void do_nothing_rodata(void)
>         __attribute__((section(".rodata")))
> {
>         return;
> }
>
> David



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1336793

On Wednesday 17 February 2016 22:44:12 Arnd Bergmann wrote:
> On Tuesday 16 February 2016 13:49:04 Kees Cook wrote:
> >  }
> >  
> > +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
> > +do_nothing_rodata(void)
> > +{
> > +       return;
> > +}
> > +
> >  static void do_nothing(void)
> >  {
> > 
> 
> I think this also needs to be marked "noinline" to ensure that the
> function does not get eliminated. I've seen clang do that on
> execute_location() recently and submitted a patch for that.

Nevermind, this gets passed by reference into execute_location(),
so it's enough if that is marked noinline, but do_nothing_rodata
needs no such annotation.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1336794

On Tuesday 16 February 2016 13:49:04 Kees Cook wrote:
>  }
>  
> +static void __attribute__((__section__(".rodata,\"a\",@progbits#")))
> +do_nothing_rodata(void)
> +{
> +       return;
> +}
> +
>  static void do_nothing(void)
>  {
> 

I think this also needs to be marked "noinline" to ensure that the
function does not get eliminated. I've seen clang do that on
execute_location() recently and submitted a patch for that.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web