Re: Problems with pointer offsets on ARM32

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Sep 15, 2020 at 12:26 AM Luka Oreskovic
<luka.oreskovic@sartura.hr> wrote:
>
> On Mon, Sep 14, 2020 at 7:49 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Sep 14, 2020 at 12:55 AM Luka Oreskovic
> > <luka.oreskovic@sartura.hr> wrote:
> > >
> > > On Fri, Sep 11, 2020 at 8:14 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Fri, Sep 11, 2020 at 9:45 AM Luka Oreskovic
> > > > <luka.oreskovic@sartura.hr> wrote:
> > > > >
> > > > > Greetings everyone,
> > > > >
> > > > > I have been testing various BPF programs on the ARM32 architecture and
> > > > > have encountered a strange error.
> > > > >
> > > > > When trying to run a simple program that prints out the arguments of
> > > > > the open syscall,
> > > > > I found some strange behaviour with the pointer offsets when accessing
> > > > > the arguments:
> > > > > The output of llvm-objdump differed from the verifier error dump log.
> > > > > Notice the differences in lines 0 and 1. Why is the bytecode being
> > > > > altered at runtime?
> > > > >
> > > > > I attached the program, the llvm-objdump result and the verifier dump below.
> > > > >
> > > > > Best wishes,
> > > > > Luka Oreskovic
> > > > >
> > > > > BPF program
> > > > > --------------------------------------------
> > > > > #include "vmlinux.h"
> > > > > #include <bpf/bpf_helpers.h>
> > > > >
> > > > > SEC("tracepoint/syscalls/sys_enter_open")
> > > > > int tracepoint__syscalls__sys_enter_open(struct trace_event_raw_sys_enter* ctx)
> > > > > {
> > > > >         const char *arg1 = (const char *)ctx->args[0];
> > > > >         int arg2 = (int)ctx->args[1];

Luka, can you apply the changes below to bpf_core_read.h header and
read these args using BPF_CORE_READ() macro:

const char *arg1 = (const char *)BPF_CORE_READ(ctx, args[0]);
int arg2 = BPF_CORE_READ(ctx, args[1]);

I'm curious if that will work (unfortunately I don't have a complete
enough setup to test this).

The patch is as follows (with broken tab<->space conversion, so please
make changes by hand):

diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index bbcefb3ff5a5..fee6328d36c0 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -261,14 +261,16 @@ enum bpf_enum_value_kind {
 #define ___type(...) typeof(___arrow(__VA_ARGS__))

 #define ___read(read_fn, dst, src_type, src, accessor)                     \
-       read_fn((void *)(dst), sizeof(*(dst)), &((src_type)(src))->accessor)
+       read_fn((void *)(dst),                                              \
+               bpf_core_field_size(((src_type)(src))->accessor),           \
+               &((src_type)(src))->accessor)

 /* "recursively" read a sequence of inner pointers using local __t var */
 #define ___rd_first(src, a) ___read(bpf_core_read, &__t, ___type(src), src, a);
 #define ___rd_last(...)
             \
        ___read(bpf_core_read, &__t,                                        \
                ___type(___nolast(__VA_ARGS__)), __t, ___last(__VA_ARGS__));
-#define ___rd_p1(...) const void *__t; ___rd_first(__VA_ARGS__)
+#define ___rd_p1(...) const void *__t = (void *)0; ___rd_first(__VA_ARGS__)
 #define ___rd_p2(...) ___rd_p1(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
 #define ___rd_p3(...) ___rd_p2(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)
 #define ___rd_p4(...) ___rd_p3(___nolast(__VA_ARGS__)) ___rd_last(__VA_ARGS__)



BTW, this approach should work for reading pointers as well, it would
be nice if you can test that as well. E.g., something like the
following:

struct task_struct *t = (void *)bpf_get_current_task();
int ppid = BPF_CORE_READ(t, group_leader, tgid);

If you try it without the patch above, it should either read garbage
or zero, but not a valid parent PID (please verify that as well).

I really appreciate your help with testing, thanks!


> > > > >
> > > > >         bpf_printk("Open arg 1: %s\n", arg1);
> > > > >         bpf_printk("Open arg 2: %d\n", arg2);
> > > > >
> > > > >         return 0;
> > > > > }
> > > > >
> > > > > char LICENSE[] SEC("license") = "GPL";
> > > > >
> > > > >

[...]

>
> Best wishes,
> Luka Oreskovic