netdev - Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 15 Jun 2020 20:55:26 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Lorenz Bauer <lmb@...udflare.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Sitnicki <jakub@...udflare.com>,
        kernel-team <kernel-team@...udflare.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags

On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@...udflare.com> wrote:
>
> On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@...udflare.com> wrote:
> > >
> > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@...udflare.com>
> > > Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
> > > ---
> > >  kernel/bpf/net_namespace.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> > > index 78cf061f8179..56133e78ae4f 100644
> > > --- a/kernel/bpf/net_namespace.c
> > > +++ b/kernel/bpf/net_namespace.c
> > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > >         struct net *net;
> > >         int ret;
> > >
> > > +       if (attr->attach_flags || attr->target_fd)
> > > +               return -EINVAL;
> > > +
> >
> > In theory it makes sense, but how did you test it?
>
> Not properly it seems, sorry!
>
> > test_progs -t flow
> > fails 5 tests.
>
> I spent today digging through this, and the issue is actually more annoying than
> I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> program being detached is actually what user space expects. We actually have
> tests that set attach_bpf_fd for these to attach points, which tells
> me that this is
> an easy mistake to make.
>
> Unfortunately I can't come up with a good fix that seems backportable:
> - Making sockmap and flow_dissector have the same semantics as cgroup
>   and lirc2 requires a bunch of changes (probably a new function for sockmap)

making flow dissector pass prog_fd as cg and lirc is certainly my preference.
Especially since tests are passing fd user code is likely doing the same,
so breakage is unlikely. Also it wasn't done that long ago, so
we can backport far enough.
It will remove cap_net_admin ugly check in bpf_prog_detach()
which is the only exception now in cap model.