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 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]

#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