netdev - Re: [PATCH bpf-next 2/2] selftests/bpf: uprobe tests should verify param/return values

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Apr 2022 17:14:31 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: uprobe tests should verify
 param/return values

On Tue, Apr 5, 2022 at 2:46 PM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> uprobe/uretprobe tests don't do any validation of arguments/return values,
> and without this we can't be sure we are attached to the right function,
> or that we are indeed attached to a uprobe or uretprobe.  To fix this
> validate argument and return value for auto-attached functions.
>
> Suggested-by: Andrii Nakryiko <andrii@...nel.org>
> Signed-off-by: Alan Maguire <alan.maguire@...cle.com>
> ---
>  .../selftests/bpf/prog_tests/uprobe_autoattach.c   | 16 ++++++++++----
>  .../selftests/bpf/progs/test_uprobe_autoattach.c   | 25 +++++++++++++++-------
>  2 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> index 03b15d6..ff85f1f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> @@ -5,14 +5,16 @@
>  #include "test_uprobe_autoattach.skel.h"
>
>  /* uprobe attach point */
> -static void autoattach_trigger_func(void)
> +static noinline int autoattach_trigger_func(int arg)
>  {
> -       asm volatile ("");

don't remove asm volatile, with static functions compiler can still do
function transformations and stuff. From GCC documentation for
noinline attribute ([0]):

  noinline

      This function attribute prevents a function from being
considered for inlining. If the function does not have side effects,
there are optimizations other than inlining that cause function calls
to be optimized away, although the function call is live. To keep such
calls from being optimized away, put

      asm ("");

  [0] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

So I suppose noinline + asm volatile is the most safe combination :)
for static functions

> +       return arg + 1;
>  }
>
>  void test_uprobe_autoattach(void)
>  {
>         struct test_uprobe_autoattach *skel;
> +       int trigger_val = 100;
> +       size_t malloc_sz = 1;
>         char *mem;
>
>         skel = test_uprobe_autoattach__open_and_load();

[...]

>
> -SEC("uretprobe/libc.so.6:free")
> +SEC("uretprobe/libc.so.6:getpid")
>  int handle_uretprobe_byname2(struct pt_regs *ctx)
>  {
> -       uretprobe_byname2_res = 4;
> +       if (PT_REGS_RC_CORE(ctx) == uretprobe_byname2_rc)
> +               uretprobe_byname2_res = 4;

Instead of checking equality on BPF side, it's more convenient to
record the actual captured value into global variable and let
user-space part check and log it properly. So if something goes wrong,
we don't just have matched/not matched flag, but we see an actual
value captured.


With that, let's better switch uretprobe from free to malloc (we
additionally check that uprobe and uretprobe can coexist properly on
the same function) and capture returned pointer from malloc(). We can
then compare that pointer to the mem variable. How cool is that? :)

>         return 0;
>  }
>
> --
> 1.8.3.1
>