netdev - Re: [PATCH bpf-next v2 1/2] bpf: allow rewriting to ports under ip_unprivileged_port

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]

Message-ID: <CAKH8qBuz2uVO2oB3rDMqcw41FOWbx5HS0vUPT2KLv_6rhZuyrw@mail.gmail.com>
Date:   Mon, 25 Jan 2021 15:32:53 -0800
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next v2 1/2] bpf: allow rewriting to ports under ip_unprivileged_port_start

On Mon, Jan 25, 2021 at 3:25 PM Martin KaFai Lau <kafai@...com> wrote:
>
> On Mon, Jan 25, 2021 at 09:26:40AM -0800, Stanislav Fomichev wrote:
> > At the moment, BPF_CGROUP_INET{4,6}_BIND hooks can rewrite user_port
> > to the privileged ones (< ip_unprivileged_port_start), but it will
> > be rejected later on in the __inet_bind or __inet6_bind.
> >
> > Let's export 'port_changed' event from the BPF program and bypass
> > ip_unprivileged_port_start range check when we've seen that
> > the program explicitly overrode the port. This is accomplished
> > by generating instructions to set ctx->port_changed along with
> > updating ctx->user_port.
> The description requires an update.
Ah, sure, will update it.

> [ ... ]
>
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index da649f20d6b2..cdf3c7e611d9 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -1055,6 +1055,8 @@ EXPORT_SYMBOL(__cgroup_bpf_run_filter_sk);
> >   * @uaddr: sockaddr struct provided by user
> >   * @type: The type of program to be exectuted
> >   * @t_ctx: Pointer to attach type specific context
> > + * @flags: Pointer to u32 which contains higher bits of BPF program
> > + *         return value (OR'ed together).
> >   *
> >   * socket is expected to be of type INET or INET6.
> >   *
> > @@ -1064,7 +1066,8 @@ EXPORT_SYMBOL(__cgroup_bpf_run_filter_sk);
> >  int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> >                                     struct sockaddr *uaddr,
> >                                     enum bpf_attach_type type,
> > -                                   void *t_ctx)
> > +                                   void *t_ctx,
> > +                                   u32 *flags)
> >  {
> >       struct bpf_sock_addr_kern ctx = {
> >               .sk = sk,
> > @@ -1087,7 +1090,8 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> >       }
> >
> >       cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
> > -     ret = BPF_PROG_RUN_ARRAY(cgrp->bpf.effective[type], &ctx, BPF_PROG_RUN);
> > +     ret = BPF_PROG_RUN_ARRAY_FLAGS(cgrp->bpf.effective[type], &ctx,
> > +                                    BPF_PROG_RUN, flags);
> >
> >       return ret == 1 ? 0 : -EPERM;
> >  }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index d0eae51b31e4..ef7c3ca53214 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -7986,6 +7986,11 @@ static int check_return_code(struct bpf_verifier_env *env)
> >                   env->prog->expected_attach_type == BPF_CGROUP_INET4_GETSOCKNAME ||
> >                   env->prog->expected_attach_type == BPF_CGROUP_INET6_GETSOCKNAME)
> >                       range = tnum_range(1, 1);
> > +             if (env->prog->expected_attach_type == BPF_CGROUP_INET4_BIND ||
> > +                 env->prog->expected_attach_type == BPF_CGROUP_INET6_BIND) {
> > +                     range = tnum_range(0, 3);
> > +                     enforce_attach_type_range = tnum_range(0, 3);
> It should be:
>                         enforce_attach_type_range = tnum_range(2, 3);
Hm, weren't we enforcing attach_type for bind progs from the beginning?
Also, looking at bpf_prog_attach_check_attach_type, it seems that we
care only about BPF_PROG_TYPE_CGROUP_SKB for
prog->enforce_expected_attach_type.
Am I missing something?