netdev - Re: [PATCH bpf-next v4 18/27] bpf, x64: Store properly return value for trampoline with multi func programs

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:   Tue, 31 Aug 2021 16:51:18 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        "Steven Rostedt (VMware)" <rostedt@...dmis.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>, Daniel Xu <dxu@...uu.xyz>,
        Viktor Malik <vmalik@...hat.com>
Subject: Re: [PATCH bpf-next v4 18/27] bpf, x64: Store properly return value
 for trampoline with multi func programs

On Thu, Aug 26, 2021 at 12:41 PM Jiri Olsa <jolsa@...hat.com> wrote:
>
> When we have multi func program attached, the trampoline
> switched to the function model of the multi func program.
>
> This breaks already attached standard programs, for example
> when we attach following program:
>
>   SEC("fexit/bpf_fentry_test2")
>   int BPF_PROG(test1, int a, __u64 b, int ret)
>
> the trampoline pushes on stack args 'a' and 'b' and return
> value 'ret'.
>
> When following multi func program is attached to bpf_fentry_test2:
>
>   SEC("fexit.multi/bpf_fentry_test*")
>   int BPF_PROG(test2, __u64 a, __u64 b, __u64 c, __u64 d,
>                        __u64 e, __u64 f, int ret)
>
> the trampoline takes this program model and pushes all 6 args
> and return value on stack.
>
> But we still have the original 'test1' program attached, that
> expects 'ret' value where there's 'c' argument now:
>
>   test1(a, b, c)
>
> To fix that we simply overwrite 'c' argument with 'ret' value,
> so test1 is called as expected and test2 gets called as:
>
>   test2(a, b, ret, d, e, f, ret)
>
> which is ok, because 'c' is not defined for bpf_fentry_test2
> anyway.
>

What if we change the order on the stack to be the return value first,
followed by input arguments. That would get us a bit closer to
unifying multi-trampoline and the normal one, right? BPF verifier
should be able to rewrite access to the last argument (i.e., return
value) for fexit programs to actually be at offset 0, and shift all
other arguments by 8 bytes. For fentry, if that helps to keep things
more aligned, we'd just skip the first 8 bytes on the stack and store
all the input arguments in the same offsets. So BPF verifier rewriting
logic stays consistent (except offset 0 will be disallowed).

Basically, I'm thinking how we can make normal and multi trampolines
more interoperable to remove those limitations that two
multi-trampolines can't be attached to the same function, which seems
like a pretty annoying limitation which will be easy to hit in
practice. Alexei previously proposed (as an optimization) to group all
to-be-attached functions into groups by number of arguments, so that
we can have up to 6 different trampolines tailored to actual functions
being attached. So that we don't save unnecessary extra input
arguments saving, which will be even more important once we allow more
than 6 arguments in the future.

With such logic, we should be able to split all the functions into
multiple underlying trampolines, so it seems like it should be
possible to also allow multiple multi-fentry programs to be attached
to the same function by having a separate bpf_trampoline just for
those functions. It will be just an extension of the above "just 6
trampolines" strategy to "as much as we need trampolines".

It's just a vague idea, sorry, I don't understand all the code yet.
But the limitation outlined in one of the previous patches seems very
limiting and unpleasant. I can totally see that some 24/7 running BPF
tracing app uses multi-fentry for tracing a small subset of kernel
functions non-stop, and then someone is trying to use bpftrace or
retsnoop to trace overlapping set of functions. And it immediately
fails. Very frustrating.

> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 40 ++++++++++++++++++++++++++++++-------
>  include/linux/bpf.h         |  1 +
>  kernel/bpf/trampoline.c     |  1 +
>  3 files changed, 35 insertions(+), 7 deletions(-)
>

[...]