netdev - Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL

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: <YtSCbIA+6JtRF/Ch@krava>
Date:   Sun, 17 Jul 2022 23:43:08 +0200
From:   Jiri Olsa <olsajiri@...il.com>
To:     Jiri Olsa <olsajiri@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.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>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Martynas Pumputis <m@...bda.lt>,
        Yutaro Hayakawa <yutaro.hayakawa@...valent.com>
Subject: Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip
 tests for CONFIG_X86_KERNEL_IBT

On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@...nel.org> wrote:
> > >
> > > The kprobe can be placed anywhere and user must be aware
> > > of the underlying instructions. Therefore fixing just
> > > the bpf program to 'fix' the address to match the actual
> > > function address when CONFIG_X86_KERNEL_IBT is enabled.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > ---
> > >  tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > index a587aeca5ae0..220d56b7c1dc 100644
> > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > @@ -2,6 +2,7 @@
> > >  #include <linux/bpf.h>
> > >  #include <bpf/bpf_helpers.h>
> > >  #include <bpf/bpf_tracing.h>
> > > +#include <stdbool.h>
> > >
> > >  char _license[] SEC("license") = "GPL";
> > >
> > > @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
> > >  extern const void bpf_fentry_test6 __ksym;
> > >  extern const void bpf_fentry_test7 __ksym;
> > >
> > > +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> > > +
> > >  __u64 test1_result = 0;
> > >  SEC("fentry/bpf_fentry_test1")
> > >  int BPF_PROG(test1, int a)
> > > @@ -37,7 +40,7 @@ __u64 test3_result = 0;
> > >  SEC("kprobe/bpf_fentry_test3")
> > >  int test3(struct pt_regs *ctx)
> > >  {
> > > -       __u64 addr = bpf_get_func_ip(ctx);
> > > +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
> > 
> > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > compensation for `call __fentry__`, but not for endr? Why can't we
> > compensate for endbr inside the kernel code as well? I'd imagine we
> > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > compensation (and thus always get &bpf_fentry_test3), but this
> > in-between solution seems to be the worst of both worlds?...
> 
> hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> I'll check

sorry for late follow up..

so the problem is that you can place kprobe anywhere in the function
(on instruction boundary) but the IBT adjustment of kprobe address is
made only if it's at the function entry and there's endbr instruction

and that kprobe address is what we return in helper:

  BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
  {
        struct kprobe *kp = kprobe_running();

        return kp ? (uintptr_t)kp->addr : 0;
  }

so the adjustment would work only for address at function entry, but
would be wrong for address within the function

perhaps we could add flag to kprobe to indicate the addr adjustment
was done and use it in helper

but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
leave it up to user

kprobe_multi and trampolines are different, because they can be
only at the function entry, so we can adjust the ip properly

jirka