netdev - Re: [PATCH bpf-next 3/3] selftests/bpf: add tests for user- and non-CO-RE BPF_CORE

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]

Message-ID: <CAEf4BzY4qXW_xV+0pcWPQp+Tda6BY69xgJnaA3RFxKc255rP2g@mail.gmail.com>
Date:   Mon, 4 Jan 2021 21:08:21 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Gilad Reti <gilad.reti@...il.com>
Subject: Re: [PATCH bpf-next 3/3] selftests/bpf: add tests for user- and
 non-CO-RE BPF_CORE_READ() variants

On Mon, Jan 4, 2021 at 7:46 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Fri, Dec 18, 2020 at 03:56:14PM -0800, Andrii Nakryiko wrote:
> > +
> > +/* shuffled layout for relocatable (CO-RE) reads */
> > +struct callback_head___shuffled {
> > +     void (*func)(struct callback_head___shuffled *head);
> > +     struct callback_head___shuffled *next;
> > +};
> > +
> > +struct callback_head k_probe_in = {};
> > +struct callback_head___shuffled k_core_in = {};
> > +
> > +struct callback_head *u_probe_in = 0;
> > +struct callback_head___shuffled *u_core_in = 0;
> > +
> > +long k_probe_out = 0;
> > +long u_probe_out = 0;
> > +
> > +long k_core_out = 0;
> > +long u_core_out = 0;
> > +
> > +int my_pid = 0;
> > +
> > +SEC("raw_tracepoint/sys_enter")
> > +int handler(void *ctx)
> > +{
> > +     int pid = bpf_get_current_pid_tgid() >> 32;
> > +
> > +     if (my_pid != pid)
> > +             return 0;
> > +
> > +     /* next pointers for kernel address space have to be initialized from
> > +      * BPF side, user-space mmaped addresses are stil user-space addresses
> > +      */
> > +     k_probe_in.next = &k_probe_in;
> > +     __builtin_preserve_access_index(({k_core_in.next = &k_core_in;}));
> > +
> > +     k_probe_out = (long)BPF_PROBE_READ(&k_probe_in, next, next, func);
> > +     k_core_out = (long)BPF_CORE_READ(&k_core_in, next, next, func);
> > +     u_probe_out = (long)BPF_PROBE_READ_USER(u_probe_in, next, next, func);
> > +     u_core_out = (long)BPF_CORE_READ_USER(u_core_in, next, next, func);
>
> I don't understand what the test suppose to demonstrate.
> co-re relocs work for kernel btf only.
> Are you saying that 'struct callback_head' happened to be used by user space
> process that allocated it in user memory. And that is the same struct as
> being used by the kernel? So co-re relocs that apply against the kernel
> will sort-of work against the data of user space process because
> the user space is using the same struct? That sounds convoluted.

The test itself just tests that bpf_probe_read_user() is executed, not
bpf_probe_read_kernel(). But yes, the use case is to read kernel data
structures from the user memory address space. See [0] for the last
time this was requested and justifications. It's not the first time
someone asked about the user-space variant of BPF_CORE_READ(), though
I won't be able to find the reference at this time.

  [0] https://lore.kernel.org/bpf/CANaYP3GetBKUPDfo6PqWnm3xuGs2GZjLF8Ed51Q1=Emv2J-dKg@mail.gmail.com/

> I struggle to see the point of patch 1:
> +#define bpf_core_read_user(dst, sz, src)                                   \
> +       bpf_probe_read_user(dst, sz, (const void *)__builtin_preserve_access_index(src))
>
> co-re for user structs? Aren't they uapi? No reloc is needed.

The use case in [0] above is for reading UAPI structs, passed as input
arguments to syscall. It's a pretty niche use case, but there are at
least two more-or-less valid benefits to use CO-RE with "stable" UAPI
structs:

  - handling 32-bit vs 64-bit UAPI structs uniformly;
  - handling UAPI fields that were added in later kernels, but are
missing on the earlier ones.

For the former, you'd need to compile two variants of the BPF program
(or do convoluted and inconvenient 32-bit UAPI struct re-definition
for 64-bit BPF target). For the latter... I guess you can do if/else
dance based on the kernel version. Which sucks and is inconvenient
(and kernel version checks are discouraged, it's more reliable to
detect availability of specific types and fields).

So all in all, while pretty rare and niche, seemed like a valid use
case. And easy to support while reusing all the macro logic almost
without any changes.