[RFC PATCH] kallsyms: Avoid weak references for kallsyms symbols

[RFC PATCH] kallsyms: Avoid weak references for kallsyms symbols

[Ard Biesheuvel]

kallsyms is a directory of all the symbols in the vmlinux binary, and so
creating it is somewhat of a chicken-and-egg problem, as its non-zero
size affects the layout of the binary, and therefore the values of the
symbols.

For this reason, the kernel is linked more than once, and the first pass
does not include any kallsyms data at all. For the linker to accept
this, the symbol declarations describing the kallsyms metadata are
emitted as having weak linkage, so they can remain unsatisfied. During
the subsequent passes, the weak references are satisfied by the kallsyms
metadata that was constructed based on information gathered from the
preceding passes.

Weak references lead to somewhat worse codegen, because taking their
address may need to produce NULL (if the reference was unsatisfied), and
this is not usually supported by RIP or PC relative symbol references.

Given that these references are ultimately always satisfied in the final
link, let's drop the weak annotation, and instead, provide fallback
definitions in the linker script that are only emitted if an unsatisfied
reference exists.

While at it, drop the FRV specific annotation that these symbols reside
in .rodata - FRV is long gone.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Fangrui Song <maskray@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 include/asm-generic/vmlinux.lds.h |  9 +++++++
 kernel/kallsyms.c                 |  6 -----
 kernel/kallsyms_internal.h        | 25 +++++++-------------
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index d1f57e4868ed341d..dd42c0fcad2b519f 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -460,6 +460,15 @@
  */
 #define RO_DATA(align)							\
 	. = ALIGN((align));						\
+	PROVIDE(kallsyms_addresses = .);				\
+	PROVIDE(kallsyms_offsets = .);					\
+	PROVIDE(kallsyms_names = .);					\
+	PROVIDE(kallsyms_num_syms = .);					\
+	PROVIDE(kallsyms_relative_base = .);				\
+	PROVIDE(kallsyms_token_table = .);				\
+	PROVIDE(kallsyms_token_index = .);				\
+	PROVIDE(kallsyms_markers = .);					\
+	PROVIDE(kallsyms_seqs_of_names = .);				\
 	.rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
 		__start_rodata = .;					\
 		*(.rodata) *(.rodata.*)					\
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 77747391f49b66cb..5b16009ee53aa05b 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -331,12 +331,6 @@ static unsigned long get_symbol_pos(unsigned long addr,
 	unsigned long symbol_start = 0, symbol_end = 0;
 	unsigned long i, low, high, mid;
 
-	/* This kernel should never had been booted. */
-	if (!IS_ENABLED(CONFIG_KALLSYMS_BASE_RELATIVE))
-		BUG_ON(!kallsyms_addresses);
-	else
-		BUG_ON(!kallsyms_offsets);
-
 	/* Do a binary search on the sorted kallsyms_addresses array. */
 	low = 0;
 	high = kallsyms_num_syms;
diff --git a/kernel/kallsyms_internal.h b/kernel/kallsyms_internal.h
index 27fabdcc40f57931..cf4124dbcc5b6d0e 100644
--- a/kernel/kallsyms_internal.h
+++ b/kernel/kallsyms_internal.h
@@ -8,24 +8,17 @@
  * These will be re-linked against their real values
  * during the second link stage.
  */
-extern const unsigned long kallsyms_addresses[] __weak;
-extern const int kallsyms_offsets[] __weak;
-extern const u8 kallsyms_names[] __weak;
+extern const unsigned long kallsyms_addresses[];
+extern const int kallsyms_offsets[];
+extern const u8 kallsyms_names[];
 
-/*
- * Tell the compiler that the count isn't in the small data section if the arch
- * has one (eg: FRV).
- */
-extern const unsigned int kallsyms_num_syms
-__section(".rodata") __attribute__((weak));
-
-extern const unsigned long kallsyms_relative_base
-__section(".rodata") __attribute__((weak));
+extern const unsigned int kallsyms_num_syms;
+extern const unsigned long kallsyms_relative_base;
 
-extern const char kallsyms_token_table[] __weak;
-extern const u16 kallsyms_token_index[] __weak;
+extern const char kallsyms_token_table[];
+extern const u16 kallsyms_token_index[];
 
-extern const unsigned int kallsyms_markers[] __weak;
-extern const u8 kallsyms_seqs_of_names[] __weak;
+extern const unsigned int kallsyms_markers[];
+extern const u8 kallsyms_seqs_of_names[];
 
 #endif // LINUX_KALLSYMS_INTERNAL_H_
-- 
2.39.2

Re: [RFC PATCH] kallsyms: Avoid weak references for kallsyms symbols

[Nick Desaulniers]

On Thu, May 4, 2023 at 10:43 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> kallsyms is a directory of all the symbols in the vmlinux binary, and so
> creating it is somewhat of a chicken-and-egg problem, as its non-zero
> size affects the layout of the binary, and therefore the values of the
> symbols.
>
> For this reason, the kernel is linked more than once, and the first pass
> does not include any kallsyms data at all. For the linker to accept
> this, the symbol declarations describing the kallsyms metadata are
> emitted as having weak linkage, so they can remain unsatisfied. During
> the subsequent passes, the weak references are satisfied by the kallsyms
> metadata that was constructed based on information gathered from the
> preceding passes.
>
> Weak references lead to somewhat worse codegen, because taking their
> address may need to produce NULL (if the reference was unsatisfied), and
> this is not usually supported by RIP or PC relative symbol references.
>
> Given that these references are ultimately always satisfied in the final
> link, let's drop the weak annotation, and instead, provide fallback
> definitions in the linker script that are only emitted if an unsatisfied
> reference exists.
>
> While at it, drop the FRV specific annotation that these symbols reside
> in .rodata - FRV is long gone.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Fangrui Song <maskray@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>

Thanks for the patch.  I did some quick boot tests of this with:
- x86 defconfig + CONFIG_KALLSYMS_SELFTEST=y
- x86 defconfig + CONFIG_KALLSYMS_SELFTEST=y + CONFIG_LTO_CLANG_THIN=y
- arm64 defconfig + CONFIG_KALLSYMS_SELFTEST=y

Curiously, I only see:
[    1.002200] kallsyms_selftest: start

in the output (when grepping for kallsyms_selftest as instructed by
the help text for KALLSYMS_SELFTEST in init/Kconfig). But that happens
regardless of this patch.

I did not test backtraces or live patching (seems like kallsyms is
related to those reading through the help texts in init/Kconfig), or
measure for binary changes.

Tested-by: Nick Desaulniers <ndesaulniers@google.com> # Boot

Based on my interpretation of
https://sourceware.org/binutils/docs/ld/PROVIDE.html, this LGTM.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  include/asm-generic/vmlinux.lds.h |  9 +++++++
>  kernel/kallsyms.c                 |  6 -----
>  kernel/kallsyms_internal.h        | 25 +++++++-------------
>  3 files changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index d1f57e4868ed341d..dd42c0fcad2b519f 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -460,6 +460,15 @@
>   */
>  #define RO_DATA(align)                                                 \
>         . = ALIGN((align));                                             \
> +       PROVIDE(kallsyms_addresses = .);                                \
> +       PROVIDE(kallsyms_offsets = .);                                  \
> +       PROVIDE(kallsyms_names = .);                                    \
> +       PROVIDE(kallsyms_num_syms = .);                                 \
> +       PROVIDE(kallsyms_relative_base = .);                            \
> +       PROVIDE(kallsyms_token_table = .);                              \
> +       PROVIDE(kallsyms_token_index = .);                              \
> +       PROVIDE(kallsyms_markers = .);                                  \
> +       PROVIDE(kallsyms_seqs_of_names = .);                            \
>         .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {           \
>                 __start_rodata = .;                                     \
>                 *(.rodata) *(.rodata.*)                                 \
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 77747391f49b66cb..5b16009ee53aa05b 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -331,12 +331,6 @@ static unsigned long get_symbol_pos(unsigned long addr,
>         unsigned long symbol_start = 0, symbol_end = 0;
>         unsigned long i, low, high, mid;
>
> -       /* This kernel should never had been booted. */
> -       if (!IS_ENABLED(CONFIG_KALLSYMS_BASE_RELATIVE))
> -               BUG_ON(!kallsyms_addresses);
> -       else
> -               BUG_ON(!kallsyms_offsets);

Even previously with weak definitions, wouldn't these values always be true?

> -
>         /* Do a binary search on the sorted kallsyms_addresses array. */
>         low = 0;
>         high = kallsyms_num_syms;
> diff --git a/kernel/kallsyms_internal.h b/kernel/kallsyms_internal.h
> index 27fabdcc40f57931..cf4124dbcc5b6d0e 100644
> --- a/kernel/kallsyms_internal.h
> +++ b/kernel/kallsyms_internal.h
> @@ -8,24 +8,17 @@
>   * These will be re-linked against their real values
>   * during the second link stage.
>   */
> -extern const unsigned long kallsyms_addresses[] __weak;
> -extern const int kallsyms_offsets[] __weak;
> -extern const u8 kallsyms_names[] __weak;
> +extern const unsigned long kallsyms_addresses[];
> +extern const int kallsyms_offsets[];
> +extern const u8 kallsyms_names[];
>
> -/*
> - * Tell the compiler that the count isn't in the small data section if the arch
> - * has one (eg: FRV).
> - */
> -extern const unsigned int kallsyms_num_syms
> -__section(".rodata") __attribute__((weak));
> -
> -extern const unsigned long kallsyms_relative_base
> -__section(".rodata") __attribute__((weak));
> +extern const unsigned int kallsyms_num_syms;
> +extern const unsigned long kallsyms_relative_base;
>
> -extern const char kallsyms_token_table[] __weak;
> -extern const u16 kallsyms_token_index[] __weak;
> +extern const char kallsyms_token_table[];
> +extern const u16 kallsyms_token_index[];
>
> -extern const unsigned int kallsyms_markers[] __weak;
> -extern const u8 kallsyms_seqs_of_names[] __weak;
> +extern const unsigned int kallsyms_markers[];
> +extern const u8 kallsyms_seqs_of_names[];
>
>  #endif // LINUX_KALLSYMS_INTERNAL_H_
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [RFC PATCH] kallsyms: Avoid weak references for kallsyms symbols

[Ard Biesheuvel]

On Mon, 15 May 2023 at 23:29, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Thu, May 4, 2023 at 10:43 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > kallsyms is a directory of all the symbols in the vmlinux binary, and so
> > creating it is somewhat of a chicken-and-egg problem, as its non-zero
> > size affects the layout of the binary, and therefore the values of the
> > symbols.
> >
> > For this reason, the kernel is linked more than once, and the first pass
> > does not include any kallsyms data at all. For the linker to accept
> > this, the symbol declarations describing the kallsyms metadata are
> > emitted as having weak linkage, so they can remain unsatisfied. During
> > the subsequent passes, the weak references are satisfied by the kallsyms
> > metadata that was constructed based on information gathered from the
> > preceding passes.
> >
> > Weak references lead to somewhat worse codegen, because taking their
> > address may need to produce NULL (if the reference was unsatisfied), and
> > this is not usually supported by RIP or PC relative symbol references.
> >
> > Given that these references are ultimately always satisfied in the final
> > link, let's drop the weak annotation, and instead, provide fallback
> > definitions in the linker script that are only emitted if an unsatisfied
> > reference exists.
> >
> > While at it, drop the FRV specific annotation that these symbols reside
> > in .rodata - FRV is long gone.
> >
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Fangrui Song <maskray@google.com>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
>
> Thanks for the patch.  I did some quick boot tests of this with:
> - x86 defconfig + CONFIG_KALLSYMS_SELFTEST=y
> - x86 defconfig + CONFIG_KALLSYMS_SELFTEST=y + CONFIG_LTO_CLANG_THIN=y
> - arm64 defconfig + CONFIG_KALLSYMS_SELFTEST=y
>
> Curiously, I only see:
> [    1.002200] kallsyms_selftest: start
>
> in the output (when grepping for kallsyms_selftest as instructed by
> the help text for KALLSYMS_SELFTEST in init/Kconfig). But that happens
> regardless of this patch.
>
> I did not test backtraces or live patching (seems like kallsyms is
> related to those reading through the help texts in init/Kconfig), or
> measure for binary changes.
>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com> # Boot
>
> Based on my interpretation of
> https://sourceware.org/binutils/docs/ld/PROVIDE.html, this LGTM.
>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>

Thanks!

> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  include/asm-generic/vmlinux.lds.h |  9 +++++++
> >  kernel/kallsyms.c                 |  6 -----
> >  kernel/kallsyms_internal.h        | 25 +++++++-------------
> >  3 files changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index d1f57e4868ed341d..dd42c0fcad2b519f 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -460,6 +460,15 @@
> >   */
> >  #define RO_DATA(align)                                                 \
> >         . = ALIGN((align));                                             \
> > +       PROVIDE(kallsyms_addresses = .);                                \
> > +       PROVIDE(kallsyms_offsets = .);                                  \
> > +       PROVIDE(kallsyms_names = .);                                    \
> > +       PROVIDE(kallsyms_num_syms = .);                                 \
> > +       PROVIDE(kallsyms_relative_base = .);                            \
> > +       PROVIDE(kallsyms_token_table = .);                              \
> > +       PROVIDE(kallsyms_token_index = .);                              \
> > +       PROVIDE(kallsyms_markers = .);                                  \
> > +       PROVIDE(kallsyms_seqs_of_names = .);                            \
> >         .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {           \
> >                 __start_rodata = .;                                     \
> >                 *(.rodata) *(.rodata.*)                                 \
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 77747391f49b66cb..5b16009ee53aa05b 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -331,12 +331,6 @@ static unsigned long get_symbol_pos(unsigned long addr,
> >         unsigned long symbol_start = 0, symbol_end = 0;
> >         unsigned long i, low, high, mid;
> >
> > -       /* This kernel should never had been booted. */
> > -       if (!IS_ENABLED(CONFIG_KALLSYMS_BASE_RELATIVE))
> > -               BUG_ON(!kallsyms_addresses);
> > -       else
> > -               BUG_ON(!kallsyms_offsets);
>
> Even previously with weak definitions, wouldn't these values always be true?
>

To be pedantic: this patch does not deal with weak definitions at all.
It only deals with weak *references*, which can remain unsatisfied.

In this particular case, one of these would remain unsatisfied, while
the other one needs to be defined in order for kallsyms not to crash
and burn as soon as it is used. After this change, both will always be
non-NULL, hence the removal.

> > -
> >         /* Do a binary search on the sorted kallsyms_addresses array. */
> >         low = 0;
> >         high = kallsyms_num_syms;
> > diff --git a/kernel/kallsyms_internal.h b/kernel/kallsyms_internal.h
> > index 27fabdcc40f57931..cf4124dbcc5b6d0e 100644
> > --- a/kernel/kallsyms_internal.h
> > +++ b/kernel/kallsyms_internal.h
> > @@ -8,24 +8,17 @@
> >   * These will be re-linked against their real values
> >   * during the second link stage.
> >   */
> > -extern const unsigned long kallsyms_addresses[] __weak;
> > -extern const int kallsyms_offsets[] __weak;
> > -extern const u8 kallsyms_names[] __weak;
> > +extern const unsigned long kallsyms_addresses[];
> > +extern const int kallsyms_offsets[];
> > +extern const u8 kallsyms_names[];
> >
> > -/*
> > - * Tell the compiler that the count isn't in the small data section if the arch
> > - * has one (eg: FRV).
> > - */
> > -extern const unsigned int kallsyms_num_syms
> > -__section(".rodata") __attribute__((weak));
> > -
> > -extern const unsigned long kallsyms_relative_base
> > -__section(".rodata") __attribute__((weak));
> > +extern const unsigned int kallsyms_num_syms;
> > +extern const unsigned long kallsyms_relative_base;
> >
> > -extern const char kallsyms_token_table[] __weak;
> > -extern const u16 kallsyms_token_index[] __weak;
> > +extern const char kallsyms_token_table[];
> > +extern const u16 kallsyms_token_index[];
> >
> > -extern const unsigned int kallsyms_markers[] __weak;
> > -extern const u8 kallsyms_seqs_of_names[] __weak;
> > +extern const unsigned int kallsyms_markers[];
> > +extern const u8 kallsyms_seqs_of_names[];
> >
> >  #endif // LINUX_KALLSYMS_INTERNAL_H_
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

Re: [RFC PATCH] kallsyms: Avoid weak references for kallsyms symbols

[Kees Cook]

On Thu, May 04, 2023 at 07:43:20PM +0200, Ard Biesheuvel wrote:
> kallsyms is a directory of all the symbols in the vmlinux binary, and so
> creating it is somewhat of a chicken-and-egg problem, as its non-zero
> size affects the layout of the binary, and therefore the values of the
> symbols.
> 
> For this reason, the kernel is linked more than once, and the first pass
> does not include any kallsyms data at all. For the linker to accept
> this, the symbol declarations describing the kallsyms metadata are
> emitted as having weak linkage, so they can remain unsatisfied. During
> the subsequent passes, the weak references are satisfied by the kallsyms
> metadata that was constructed based on information gathered from the
> preceding passes.
> 
> Weak references lead to somewhat worse codegen, because taking their
> address may need to produce NULL (if the reference was unsatisfied), and
> this is not usually supported by RIP or PC relative symbol references.
> 
> Given that these references are ultimately always satisfied in the final
> link, let's drop the weak annotation, and instead, provide fallback
> definitions in the linker script that are only emitted if an unsatisfied
> reference exists.
> 
> While at it, drop the FRV specific annotation that these symbols reside
> in .rodata - FRV is long gone.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Fangrui Song <maskray@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  include/asm-generic/vmlinux.lds.h |  9 +++++++
>  kernel/kallsyms.c                 |  6 -----
>  kernel/kallsyms_internal.h        | 25 +++++++-------------
>  3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index d1f57e4868ed341d..dd42c0fcad2b519f 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -460,6 +460,15 @@
>   */
>  #define RO_DATA(align)							\
>  	. = ALIGN((align));						\
> +	PROVIDE(kallsyms_addresses = .);				\
> +	PROVIDE(kallsyms_offsets = .);					\
> +	PROVIDE(kallsyms_names = .);					\
> +	PROVIDE(kallsyms_num_syms = .);					\
> +	PROVIDE(kallsyms_relative_base = .);				\
> +	PROVIDE(kallsyms_token_table = .);				\
> +	PROVIDE(kallsyms_token_index = .);				\
> +	PROVIDE(kallsyms_markers = .);					\
> +	PROVIDE(kallsyms_seqs_of_names = .);				\

Perhaps add comments here and in kernel/kallsyms_internal.h to correlate
this list and the externs? "This list is needed because ..., see the
externs in kallsyms_internal.h." or similar?

Otherwise, yeah, looks good.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook