[PATCH] kasan: account for new sections when instrumenting globals

[PATCH] kasan: account for new sections when instrumenting globals

[Dmitry Vyukov]

When I build kernel with CONFIG_KASAN and gcc6 (which instruments globals
and inserts global constructors and destructors), vmlinux contains some
new unaccounted sections: .text.exit .text.startup .dtors.
This messes vvar/percpu layout. Want:

ffffffff822bfd80 D _edata
ffffffff822c0000 D __vvar_beginning_hack
ffffffff822c0000 A __vvar_page
ffffffff822c0080 0000000000000098 D vsyscall_gtod_data
ffffffff822c1000 A __init_begin
ffffffff822c1000 D init_per_cpu__irq_stack_union
ffffffff822c1000 A __per_cpu_load
ffffffff822d3000 D init_per_cpu__gdt_page

Got:

ffffffff8279a600 D _edata
ffffffff8279b000 A __vvar_page
ffffffff8279c000 A __init_begin
ffffffff8279c000 D init_per_cpu__irq_stack_union
ffffffff8279c000 A __per_cpu_load
ffffffff8279e000 D __vvar_beginning_hack
ffffffff8279e080 0000000000000098 D vsyscall_gtod_data
ffffffff827ae000 D init_per_cpu__gdt_page

If my reading of the linker script is correct, this happens because
__vvar_page and .vvar get different addresses here:
//arch/x86/kernel/vmlinux.lds.S

	. = ALIGN(PAGE_SIZE);
	__vvar_page = .;

	.vvar : AT(ADDR(.vvar) - LOAD_OFFSET) {
		/* work around gold bug 13023 */
		__vvar_beginning_hack = .;

Merge .text.exit into EXIT_TEXT, .text.startup into INIT_TEXT and
.dtors into INIT_DATA.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>

---

I can't say I fully understand what happens here, but it fixes my
build and boot. I think we need something along these lines,
but I am not sure about details.
---
 include/asm-generic/vmlinux.lds.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6a67ab9..6067d01 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -510,6 +510,7 @@
 #define KERNEL_CTORS()	. = ALIGN(8);			   \
 			VMLINUX_SYMBOL(__ctors_start) = .; \
 			*(.ctors)			   \
+			*(.dtors)			   \
 			*(SORT(.init_array.*))		   \
 			*(.init_array)			   \
 			VMLINUX_SYMBOL(__ctors_end) = .;
@@ -542,7 +543,9 @@
 
 #define INIT_TEXT							\
 	*(.init.text)							\
-	MEM_DISCARD(init.text)
+	*(.text.startup)						\
+	MEM_DISCARD(init.text)						\
+	MEM_DISCARD(text.startup)
 
 #define EXIT_DATA							\
 	*(.exit.data)							\
@@ -551,7 +554,9 @@
 
 #define EXIT_TEXT							\
 	*(.exit.text)							\
-	MEM_DISCARD(exit.text)
+	*(.text.exit)							\
+	MEM_DISCARD(exit.text)						\
+	MEM_DISCARD(text.exit)
 
 #define EXIT_CALL							\
 	*(.exitcall.exit)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Andrey Ryabinin]

2016-06-22 20:07 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
> When I build kernel with CONFIG_KASAN and gcc6 (which instruments globals
> and inserts global constructors and destructors), vmlinux contains some
> new unaccounted sections: .text.exit .text.startup .dtors.
> This messes vvar/percpu layout. Want:
>
> ffffffff822bfd80 D _edata
> ffffffff822c0000 D __vvar_beginning_hack
> ffffffff822c0000 A __vvar_page
> ffffffff822c0080 0000000000000098 D vsyscall_gtod_data
> ffffffff822c1000 A __init_begin
> ffffffff822c1000 D init_per_cpu__irq_stack_union
> ffffffff822c1000 A __per_cpu_load
> ffffffff822d3000 D init_per_cpu__gdt_page
>
> Got:
>
> ffffffff8279a600 D _edata
> ffffffff8279b000 A __vvar_page
> ffffffff8279c000 A __init_begin
> ffffffff8279c000 D init_per_cpu__irq_stack_union
> ffffffff8279c000 A __per_cpu_load
> ffffffff8279e000 D __vvar_beginning_hack
> ffffffff8279e080 0000000000000098 D vsyscall_gtod_data
> ffffffff827ae000 D init_per_cpu__gdt_page
>
> If my reading of the linker script is correct, this happens because
> __vvar_page and .vvar get different addresses here:
> //arch/x86/kernel/vmlinux.lds.S
>
>         . = ALIGN(PAGE_SIZE);
>         __vvar_page = .;
>
>         .vvar : AT(ADDR(.vvar) - LOAD_OFFSET) {
>                 /* work around gold bug 13023 */
>                 __vvar_beginning_hack = .;
>
> Merge .text.exit into EXIT_TEXT, .text.startup into INIT_TEXT and
> .dtors into INIT_DATA.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>
> ---
>
> I can't say I fully understand what happens here, but it fixes my
> build and boot. I think we need something along these lines,
> but I am not sure about details.

Frankly, I don't understand why .text.[exit,startup] sections has any relation
to the bug you described, but maybe I just don't now linker

> ---
>  include/asm-generic/vmlinux.lds.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..6067d01 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -510,6 +510,7 @@
>  #define KERNEL_CTORS() . = ALIGN(8);                      \
>                         VMLINUX_SYMBOL(__ctors_start) = .; \
>                         *(.ctors)                          \
> +                       *(.dtors)                          \

Why destructors added to constructors? We certainly don't want invoke
destructors on startup.

Actually, we can discard dtors section (and .fini_array too), as KASAN
destructors just call  __asan_unregister_globals() which is nop,
and there is no other users of destructors in the kernel.

And how did you ended up with .dtors section anyway? I don't see it in
my build (gcc-6.1.0), and I though that recent gcc uses
.init_array and .fini_array instead of .ctor and .dtors respectively.

>                         *(SORT(.init_array.*))             \
>                         *(.init_array)                     \
>                         VMLINUX_SYMBOL(__ctors_end) = .;
> @@ -542,7 +543,9 @@
>
>  #define INIT_TEXT                                                      \
>         *(.init.text)                                                   \
> -       MEM_DISCARD(init.text)
> +       *(.text.startup)                                                \
> +       MEM_DISCARD(init.text)                                          \
> +       MEM_DISCARD(text.startup)

Why is .memtext.startup here? It shouldn't exist at all.


>  #define EXIT_DATA                                                      \
>         *(.exit.data)                                                   \
> @@ -551,7 +554,9 @@
>
>  #define EXIT_TEXT                                                      \
>         *(.exit.text)                                                   \
> -       MEM_DISCARD(exit.text)
> +       *(.text.exit)                                                   \
> +       MEM_DISCARD(exit.text)                                          \
> +       MEM_DISCARD(text.exit)
>

ditto

>  #define EXIT_CALL                                                      \
>         *(.exitcall.exit)
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Andrey Ryabinin]

2016-06-23 14:49 GMT+03:00 Andrey Ryabinin <ryabinin.a.a@gmail.com>:

> Frankly, I don't understand why .text.[exit,startup] sections has any relation
> to the bug you described, but maybe I just don't now linker

... scripting well enough to understand what's going on here.

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Dmitry Vyukov]

On Thu, Jun 23, 2016 at 1:50 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2016-06-23 14:49 GMT+03:00 Andrey Ryabinin <ryabinin.a.a@gmail.com>:
>
>> Frankly, I don't understand why .text.[exit,startup] sections has any relation
>> to the bug you described, but maybe I just don't now linker
>
> ... scripting well enough to understand what's going on here.


I noticed that these 3 new sections are what's different between
kernel that works and kernel that does not work. So I decided to
account all of them
Checked now, it works only with .dtors.
Is it preferable to add only .dtors or all sections?

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Andrey Ryabinin]

2016-06-23 15:40 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
> On Thu, Jun 23, 2016 at 1:50 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>> 2016-06-23 14:49 GMT+03:00 Andrey Ryabinin <ryabinin.a.a@gmail.com>:
>>
>>> Frankly, I don't understand why .text.[exit,startup] sections has any relation
>>> to the bug you described, but maybe I just don't now linker
>>
>> ... scripting well enough to understand what's going on here.
>
>
> I noticed that these 3 new sections are what's different between
> kernel that works and kernel that does not work. So I decided to
> account all of them
> Checked now, it works only with .dtors.
> Is it preferable to add only .dtors or all sections?

No. Merging text.startup/text.exit into INIT_TEXT/EXIT_TEXT makes sense anyway.
This will save us some memory. Also we can add .fini_array into
DISCARD section, because we don't need destructors.

As for .dtors, at first I'd like to understand where it comes from.
AFAIU it shouldn't exist at all.
Recent gcc should generate .fini_array instead of .dtors -
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770
So something is wrong here. Can you share your kernel .config, kernel version?
And you use gcc-6.1.0, right?

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Dmitry Vyukov]

On Thu, Jun 23, 2016 at 3:06 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2016-06-23 15:40 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
>> On Thu, Jun 23, 2016 at 1:50 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>>> 2016-06-23 14:49 GMT+03:00 Andrey Ryabinin <ryabinin.a.a@gmail.com>:
>>>
>>>> Frankly, I don't understand why .text.[exit,startup] sections has any relation
>>>> to the bug you described, but maybe I just don't now linker
>>>
>>> ... scripting well enough to understand what's going on here.
>>
>>
>> I noticed that these 3 new sections are what's different between
>> kernel that works and kernel that does not work. So I decided to
>> account all of them
>> Checked now, it works only with .dtors.
>> Is it preferable to add only .dtors or all sections?
>
> No. Merging text.startup/text.exit into INIT_TEXT/EXIT_TEXT makes sense anyway.
> This will save us some memory. Also we can add .fini_array into
> DISCARD section, because we don't need destructors.
>
> As for .dtors, at first I'd like to understand where it comes from.
> AFAIU it shouldn't exist at all.
> Recent gcc should generate .fini_array instead of .dtors -
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770
> So something is wrong here. Can you share your kernel .config, kernel version?
> And you use gcc-6.1.0, right?

gcc version 6.1.1 20160513

.dtors contains asan destructors for global vars:

  [18] .dtors            PROGBITS         ffffffff8279a600  016c7600
       0000000000002e20  0000000000000000  WA       0     0     8

016c7600  00 1b b4 81 ff ff ff ff  20 1b b4 81 ff ff ff ff  |........ .......|
016c7610  40 1b b4 81 ff ff ff ff  60 1b b4 81 ff ff ff ff  |@.......`.......|
016c7620  80 1b b4 81 ff ff ff ff  a0 1b b4 81 ff ff ff ff  |................|
016c7630  c0 1b b4 81 ff ff ff ff  e0 1b b4 81 ff ff ff ff  |................|

vmlinux.old:ffffffff81b41b20 0000000000000017 t
_GLOBAL__sub_D_65535_0_init_uts_ns
vmlinux.old:ffffffff81b41b40 0000000000000017 t
_GLOBAL__sub_D_65535_0_root_mountflags
vmlinux.old:ffffffff81b41b60 0000000000000017 t _GLOBAL__sub_D_65535_0_rd_prompt
vmlinux.old:ffffffff81b41b80 0000000000000017 t
_GLOBAL__sub_D_65535_0_initrd_load

ffffffff81b41b20 <_GLOBAL__sub_D_65535_0_init_uts_ns>:
ffffffff81b41b20:       55                      push   %rbp
ffffffff81b41b21:       be 03 00 00 00          mov    $0x3,%esi
ffffffff81b41b26:       48 c7 c7 a0 23 21 82    mov    $0xffffffff822123a0,%rdi
ffffffff81b41b2d:       48 89 e5                mov    %rsp,%rbp
ffffffff81b41b30:       e8 9b 85 83 ff          callq
ffffffff8137a0d0 <__asan_unregister_globals>
ffffffff81b41b35:       5d                      pop    %rbp
ffffffff81b41b36:       c3                      retq
ffffffff81b41b37:       66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
ffffffff81b41b3e:       00 00

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Dmitry Vyukov]

On Thu, Jun 23, 2016 at 3:19 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Thu, Jun 23, 2016 at 3:06 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>> 2016-06-23 15:40 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
>>> On Thu, Jun 23, 2016 at 1:50 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>>>> 2016-06-23 14:49 GMT+03:00 Andrey Ryabinin <ryabinin.a.a@gmail.com>:
>>>>
>>>>> Frankly, I don't understand why .text.[exit,startup] sections has any relation
>>>>> to the bug you described, but maybe I just don't now linker
>>>>
>>>> ... scripting well enough to understand what's going on here.
>>>
>>>
>>> I noticed that these 3 new sections are what's different between
>>> kernel that works and kernel that does not work. So I decided to
>>> account all of them
>>> Checked now, it works only with .dtors.
>>> Is it preferable to add only .dtors or all sections?
>>
>> No. Merging text.startup/text.exit into INIT_TEXT/EXIT_TEXT makes sense anyway.
>> This will save us some memory. Also we can add .fini_array into
>> DISCARD section, because we don't need destructors.
>>
>> As for .dtors, at first I'd like to understand where it comes from.
>> AFAIU it shouldn't exist at all.
>> Recent gcc should generate .fini_array instead of .dtors -
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770
>> So something is wrong here. Can you share your kernel .config, kernel version?
>> And you use gcc-6.1.0, right?
>
> gcc version 6.1.1 20160513
>
> .dtors contains asan destructors for global vars:
>
>   [18] .dtors            PROGBITS         ffffffff8279a600  016c7600
>        0000000000002e20  0000000000000000  WA       0     0     8
>
> 016c7600  00 1b b4 81 ff ff ff ff  20 1b b4 81 ff ff ff ff  |........ .......|
> 016c7610  40 1b b4 81 ff ff ff ff  60 1b b4 81 ff ff ff ff  |@.......`.......|
> 016c7620  80 1b b4 81 ff ff ff ff  a0 1b b4 81 ff ff ff ff  |................|
> 016c7630  c0 1b b4 81 ff ff ff ff  e0 1b b4 81 ff ff ff ff  |................|
>
> vmlinux.old:ffffffff81b41b20 0000000000000017 t
> _GLOBAL__sub_D_65535_0_init_uts_ns
> vmlinux.old:ffffffff81b41b40 0000000000000017 t
> _GLOBAL__sub_D_65535_0_root_mountflags
> vmlinux.old:ffffffff81b41b60 0000000000000017 t _GLOBAL__sub_D_65535_0_rd_prompt
> vmlinux.old:ffffffff81b41b80 0000000000000017 t
> _GLOBAL__sub_D_65535_0_initrd_load
>
> ffffffff81b41b20 <_GLOBAL__sub_D_65535_0_init_uts_ns>:
> ffffffff81b41b20:       55                      push   %rbp
> ffffffff81b41b21:       be 03 00 00 00          mov    $0x3,%esi
> ffffffff81b41b26:       48 c7 c7 a0 23 21 82    mov    $0xffffffff822123a0,%rdi
> ffffffff81b41b2d:       48 89 e5                mov    %rsp,%rbp
> ffffffff81b41b30:       e8 9b 85 83 ff          callq
> ffffffff8137a0d0 <__asan_unregister_globals>
> ffffffff81b41b35:       5d                      pop    %rbp
> ffffffff81b41b36:       c3                      retq
> ffffffff81b41b37:       66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
> ffffffff81b41b3e:       00 00


gcc/asan.c seems to use plain dtors:
      fn = builtin_decl_implicit (BUILT_IN_ASAN_UNREGISTER_GLOBALS);
      append_to_statement_list (build_call_expr (fn, 2,
build_fold_addr_expr (var), gcount_tree), &dtor_statements);
      cgraph_build_static_cdtor ('D', dtor_statements, priority);

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Andrey Ryabinin]

2016-06-23 16:21 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
>>
>> gcc version 6.1.1 20160513
>>
>> .dtors contains asan destructors for global vars:
>>

Something has changed very recently, because my gcc-6.1.0 2016-04-27
generate .fini_array.
So if this is not a bug in gcc, and such change done intentionally, we
can just discard .dtors.
In case this is a bug, and it will be fixed in gcc release (6.2.0) we
can ignore it.

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Dmitry Vyukov]

On Thu, Jun 23, 2016 at 4:00 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2016-06-23 16:21 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
>>>
>>> gcc version 6.1.1 20160513
>>>
>>> .dtors contains asan destructors for global vars:
>>>
>
> Something has changed very recently, because my gcc-6.1.0 2016-04-27
> generate .fini_array.
> So if this is not a bug in gcc, and such change done intentionally, we
> can just discard .dtors.
> In case this is a bug, and it will be fixed in gcc release (6.2.0) we
> can ignore it.


Okay, turns out we configure our compilers with
--disable-initfini-array for some reason. This switches it to usage of
.ctors/.dtors.

What about?

--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -537,15 +537,19 @@

 #define INIT_TEXT                                                      \
        *(.init.text)                                                   \
+       *(.text.startup)                                                \
        MEM_DISCARD(init.text)

 #define EXIT_DATA                                                      \
        *(.exit.data)                                                   \
+       *(.fini_array)                                                  \
+       *(.dtors)                                                       \
        MEM_DISCARD(exit.data)                                          \
        MEM_DISCARD(exit.rodata)

 #define EXIT_TEXT                                                      \
        *(.exit.text)                                                   \
+       *(.text.exit)                                                   \
        MEM_DISCARD(exit.text)

 #define EXIT_CALL                                                      \

Re: [PATCH] kasan: account for new sections when instrumenting globals

[Andrey Ryabinin]

2016-06-23 17:38 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
> On Thu, Jun 23, 2016 at 4:00 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>> 2016-06-23 16:21 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
>>>>
>>>> gcc version 6.1.1 20160513
>>>>
>>>> .dtors contains asan destructors for global vars:
>>>>
>>
>> Something has changed very recently, because my gcc-6.1.0 2016-04-27
>> generate .fini_array.
>> So if this is not a bug in gcc, and such change done intentionally, we
>> can just discard .dtors.
>> In case this is a bug, and it will be fixed in gcc release (6.2.0) we
>> can ignore it.
>
>
> Okay, turns out we configure our compilers with
> --disable-initfini-array for some reason. This switches it to usage of
> .ctors/.dtors.
>
> What about?
>

Looks good.

> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -537,15 +537,19 @@
>
>  #define INIT_TEXT                                                      \
>         *(.init.text)                                                   \
> +       *(.text.startup)                                                \
>         MEM_DISCARD(init.text)
>
>  #define EXIT_DATA                                                      \
>         *(.exit.data)                                                   \
> +       *(.fini_array)                                                  \
> +       *(.dtors)                                                       \
>         MEM_DISCARD(exit.data)                                          \
>         MEM_DISCARD(exit.rodata)
>
>  #define EXIT_TEXT                                                      \
>         *(.exit.text)                                                   \
> +       *(.text.exit)                                                   \
>         MEM_DISCARD(exit.text)
>
>  #define EXIT_CALL                                                      \