linux-kernel - Re: [PATCH v5] kallsyms: Avoid weak references for kallsyms symbols

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXERSHjh0k9uCmYuNf31Fg79sd_6EHuS=Y_-xEdNGWeiAw@mail.gmail.com>
Date: Tue, 23 Apr 2024 18:22:10 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Ard Biesheuvel <ardb+git@...gle.com>, 
	linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org, 
	Nick Desaulniers <ndesaulniers@...gle.com>, Kees Cook <keescook@...omium.org>, 
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v5] kallsyms: Avoid weak references for kallsyms symbols

On Tue, 23 Apr 2024 at 18:01, Konrad Dybcio <konrad.dybcio@...aro.org> wrote:
>
>
>
> On 4/22/24 18:02, Masahiro Yamada wrote:
> > On Sat, Apr 20, 2024 at 11:53 PM Ard Biesheuvel <ardb+git@...gle.com> wrote:
> >>
> >> From: Ard Biesheuvel <ardb@...nel.org>
> >>
> >> kallsyms is a directory of all the symbols in the vmlinux binary, and so
> >> creating it poses 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 on the declarations, and instead,
> >> provide fallback definitions with weak linkage. This informs the
> >> compiler that ultimately, the reference will always be satisfied.
> >>
> >> While at it, drop the FRV specific annotation that these symbols reside
> >> in .rodata - FRV is long gone.
> >>
> >> Cc: Masahiro Yamada <masahiroy@...nel.org>
> >> Cc: linux-kbuild@...r.kernel.org
> >> Acked-by: Nick Desaulniers <ndesaulniers@...gle.com>
> >> Acked-by: Kees Cook <keescook@...omium.org>
> >> Acked-by: Arnd Bergmann <arnd@...db.de>
> >> Link: https://lore.kernel.org/all/20240415075837.2349766-5-ardb+git@google.com
> >> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
> >> ---
> >> v5: - avoid PROVIDE() in the linker script, use weak definitions instead
> >>      - drop tested-by, replace reviewed-by with acked-by
> >>
> >
> > Applied to linux-kbuild. Thanks.
>
> Hi, this commit seems to break call traces, resulting in output like:
>
> [    2.777006] Call trace:
> [    2.777007]  _text+0x89e7e8/0x39e0000
> [    2.777008]  _text+0x89e82c/0x39e0000
> [    2.777009]  _text+0x2b940cc/0x2bd2a90
> [    2.777011]  _text+0x2b941a4/0x2bd2a90
> [    2.777012]  _text+0x145dc/0x39e0000
> [    2.777014]  _text+0x2b51184/0x2bd2a90
> [    2.777016]  _text+0x18fc6a4/0x39e0000
> [    2.777018]  _text+0x15644/0x39e0000
> [    2.777019] ---[ end trace 0000000000000000 ]---
>

This patch triggers an issue in the compiler, which appears to perform
constant propagation on variables defined as weak, and this is
definitely a compiler bug. (A weak variable can be superseded by
another instance from a different object at link time, so the compiler
cannot make assumptions based on the version of the variable it
observes at compile time)

It has already been dropped from the kbuild tree.