linux-kernel - Re: bpftool does not print full names with LLVM 17 and newer

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: <CAADnVQ+YXf=1iO3C7pBvV1vhfWDyko2pJzKDXv7i6fkzsBM0ig@mail.gmail.com>
Date: Fri, 17 May 2024 16:33:03 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Ivan Babrou <ivan@...udflare.com>
Cc: bpf <bpf@...r.kernel.org>, kernel-team <kernel-team@...udflare.com>, 
	linux-kernel <linux-kernel@...r.kernel.org>, clang-built-linux <llvm@...ts.linux.dev>
Subject: Re: bpftool does not print full names with LLVM 17 and newer

On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@...udflare.com> wrote:
>
> Hello,
>
> We recently bumped LLVM used for bpftool compilation from 15 to 18 and
> our alerting system notified us about some unknown bpf programs. It
> turns out, the names were truncated to 15 chars, whereas before they
> were longer.
>
> After some investigation, I was able to see that the following code:
>
>     diff --git a/src/common.c b/src/common.c
>     index 958e92a..ac38506 100644
>     --- a/src/common.c
>     +++ b/src/common.c
>     @@ -435,7 +435,9 @@ void get_prog_full_name(const struct
> bpf_prog_info *prog_info, int prog_fd,
>         if (!prog_btf)
>             goto copy_name;
>
>     +    printf("[0] finfo.type_id = %x\n", finfo.type_id);
>         func_type = btf__type_by_id(prog_btf, finfo.type_id);
>     +    printf("[1] finfo.type_id = %x\n", finfo.type_id);
>         if (!func_type || !btf_is_func(func_type))
>             goto copy_name;
>
> When ran under gdb, shows:
>
>     (gdb) b common.c:439
>     Breakpoint 1 at 0x16859: file common.c, line 439.
>
>     (gdb) r
>     3403: tracing  [0] finfo.type_id = 0
>
>     Breakpoint 1, get_prog_full_name (prog_info=0x7fffffffe160,
> prog_fd=3, name_buff=0x7fffffffe030 "", buff_len=128) at common.c:439
>     439        func_type = btf__type_by_id(prog_btf, finfo.type_id);
>     (gdb) print finfo
>     $1 = {insn_off = 0, type_id = 1547}
>
>
> Notice that finfo.type_id is printed as zero, but in gdb it is in fact 1547.
>
> Disassembly difference looks like this:
>
>     -    8b 75 cc                 mov    -0x34(%rbp),%esi
>     -    e8 47 8d 02 00           call   3f5b0 <btf__type_by_id>
>     +    31 f6                    xor    %esi,%esi
>     +    e8 a9 8c 02 00           call   3f510 <btf__type_by_id>
>
> This can be avoided if one removes "const" during finfo initialization:
>
>     const struct bpf_func_info finfo = {};
>
> This seems like a pretty annoying miscompilation, and hopefully
> there's a way to make clang complain about this loudly, but that's
> outside of my expertise. There might be other places like this that we
> just haven't noticed yet.
>
> I can send a patch to fix this particular issue, but I'm hoping for a
> more comprehensive approach from people who know better.

Wow. Great catch. Please send a patch to fix bpftool and,
I agree, llvm should be warning about such footgun,
but the way ptr_to_u64() is written is probably silencing it.
We probably should drop 'const' from it:
static inline __u64 ptr_to_u64(const void *ptr)

and maybe add a flavor of ptr_to_u64 with extra check
that the arg doesn't have a const modifier.
__builtin_types_compatible_p(typeof(ptr), void *)
should do the trick.