netdev - Re: [PATCH bpf-next v6 10/10] selftests/bpf: verify lsm

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: <CAEf4Bzb0Dnh49rwy8eFwoZK1ThOn-YQjcwXJiKbT-p7aATqEQw@mail.gmail.com>
Date:   Wed, 11 May 2022 20:37:49 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf-next v6 10/10] selftests/bpf: verify lsm_cgroup struct
 sock access

On Tue, May 10, 2022 at 10:31 AM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On Mon, May 9, 2022 at 4:44 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Mon, May 9, 2022 at 4:38 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > On Mon, May 9, 2022 at 2:54 PM Andrii Nakryiko
> > > <andrii.nakryiko@...il.com> wrote:
> > > >
> > > > On Fri, Apr 29, 2022 at 2:16 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > > > >
> > > > > sk_priority & sk_mark are writable, the rest is readonly.
> > > > >
> > > > > Add new ldx_offset fixups to lookup the offset of struct field.
> > > > > Allow using test.kfunc regardless of prog_type.
> > > > >
> > > > > One interesting thing here is that the verifier doesn't
> > > > > really force me to add NULL checks anywhere :-/
> > > > >
> > > > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > > > ---
> > > > >  tools/testing/selftests/bpf/test_verifier.c   | 54 ++++++++++++++++++-
> > > > >  .../selftests/bpf/verifier/lsm_cgroup.c       | 34 ++++++++++++
> > > > >  2 files changed, 87 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 tools/testing/selftests/bpf/verifier/lsm_cgroup.c
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/tools/testing/selftests/bpf/verifier/lsm_cgroup.c b/tools/testing/selftests/bpf/verifier/lsm_cgroup.c
> > > > > new file mode 100644
> > > > > index 000000000000..af0efe783511
> > > > > --- /dev/null
> > > > > +++ b/tools/testing/selftests/bpf/verifier/lsm_cgroup.c
> > > > > @@ -0,0 +1,34 @@
> > > > > +#define SK_WRITABLE_FIELD(tp, field, size, res) \
> > > > > +{ \
> > > > > +       .descr = field, \
> > > > > +       .insns = { \
> > > > > +               /* r1 = *(u64 *)(r1 + 0) */ \
> > > > > +               BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0), \
> > > > > +               /* r1 = *(u64 *)(r1 + offsetof(struct socket, sk)) */ \
> > > > > +               BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0), \
> > > > > +               /* r2 = *(u64 *)(r1 + offsetof(struct sock, <field>)) */ \
> > > > > +               BPF_LDX_MEM(size, BPF_REG_2, BPF_REG_1, 0), \
> > > > > +               /* *(u64 *)(r1 + offsetof(struct sock, <field>)) = r2 */ \
> > > > > +               BPF_STX_MEM(size, BPF_REG_1, BPF_REG_2, 0), \
> > > > > +               BPF_MOV64_IMM(BPF_REG_0, 1), \
> > > > > +               BPF_EXIT_INSN(), \
> > > > > +       }, \
> > > > > +       .result = res, \
> > > > > +       .errstr = res ? "no write support to 'struct sock' at off" : "", \
> > > > > +       .prog_type = BPF_PROG_TYPE_LSM, \
> > > > > +       .expected_attach_type = BPF_LSM_CGROUP, \
> > > > > +       .kfunc = "socket_post_create", \
> > > > > +       .fixup_ldx = { \
> > > > > +               { "socket", "sk", 1 }, \
> > > > > +               { tp, field, 2 }, \
> > > > > +               { tp, field, 3 }, \
> > > > > +       }, \
> > > > > +}
> > > > > +
> > > > > +SK_WRITABLE_FIELD("sock_common", "skc_family", BPF_H, REJECT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_sndtimeo", BPF_DW, REJECT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_priority", BPF_W, ACCEPT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_mark", BPF_W, ACCEPT),
> > > > > +SK_WRITABLE_FIELD("sock", "sk_pacing_rate", BPF_DW, REJECT),
> > > > > +
> > > >
> > > > have you tried writing it as C program and adding the test to
> > > > test_progs? Does something not work there?
> > >
> > > Seems like it should work, I don't see any issues with writing 5
> > > programs to test each field.
> > > But test_verified still feels like a better fit? Any reason in
> > > particular you'd prefer test_progs over test_verifier?
> >
> > Adding that fixup_ldx->strct special handling didn't feel like the
> > best fit, tbh. test_progs is generally much nicer to deal with in
> > terms of CI and in terms of comprehending what's going on and
> > supporting the code longer term.
>
> This is not new, right? We already have a bunch of fixup_xxx things.

I'm not saying it's wrong, but we don't have to keep adding extra
custom fixup_xxx things and having hand crafted assembly test cases if
we can do C tests, right? BPF assembly tests are sometimes necessary
if we need to craft some special conditions which are hard to
guarantee from Clang side during C to BPF assembly translation. But
this one doesn't seem to be the case.

> I can try to move this into test_progs in largely the same manner if
> you prefer, having a C file per field seems like an overkill.

You don't need a separate C file for each case. See what Joanne does
with SEC("?...") for dynptr tests, or what Kumar did for his kptr
tests. You can put multiple negative tests as separate BPF programs in
one file with auto-load disabled through SEC("?...") and then
open/load skeleton each time for each program, one at a time.