linux-kernel - Re: [PATCH v2] x86/lib: Do not use local symbols with SYM_CODE_START

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]

Date:   Thu, 1 Jun 2023 17:53:13 -0700
From:   Nadav Amit <nadav.amit@...il.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Dave Hansen <dave.hansen@...el.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] x86/lib: Do not use local symbols with
 SYM_CODE_START_LOCAL()

> On May 27, 2023, at 5:29 AM, Borislav Petkov <bp@...en8.de> wrote:
> 
> As to what you want to address, I'll talk to toolchain folks first and
> get back to you.

I hope you got the answer you were looking for. I am not sure what is holding
this simple patch back.

To rehash - we are talking about local two symbols that are not exposed. Based
on my search they cover the only region of the kernel text (on x86) that is
not covered by any symbol.

Doing so have two types of impacts. Some tools are affected by the fact the
closest previous symbol is not related, and as a result the symbol they show
when they unwind the stack is unrelated. So instead of seeing
“bad_get_user_clac”, you may see __get_user_nocheck_8 . This is confusing and
 misleading users.

This should impact perf, ftrace, /proc/[pid]/stack, dump_stack(), BUG(). 

The second type of impact occurs since certain code addresses is not covered
by any symbol. This mostly results in reduced functionality of tools.

This includes for instance gdb that cannot “disas” addresses in
bad_get_user_clac (you can x/i for reduced functionality) or crash in
which “dis” only disassembles a single instruction. It might also have impact
on backtraces - I did not try it.

addr2line and llvm-symbolizer also seem to be affected in such a way and they
do not find the symbol that is associated with addresses in
bad_get_user_clac. This means that tools that rely such tools, including
decode_stacktrace.sh are also affected. [*]

There might be other impacts, for instance on kpatch.

Overall, as a general rule, I think it would be best not to have code that is
not covered by any symbol. It can result in misleading output from the kernel
or related tools, and in addition in more limited functionality from tools
such as gdb and crash.

More concretely, I think these two symbols should not be stripped. The fact
that the code of these symbols runs under relatively complicated conditions
(exception tables), makes it even more important to let debug/tracing tools
to see them.

I you wish, I can include the gist of these points in the commit log, although
I think it might be an overkill.

[*] It is worth noting that tools such as addrline and llvm-symbolizer
    are able to use DWARF to find the source code location, yet they
    do not appear to be able to find the relevant symbol.