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  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:   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.

Powered by blists - more mailing lists