On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@xxxxxxxxxxxxxx> 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.
