On Mon, Jan 25, 2021 at 03:32:53PM -0800, Stanislav Fomichev wrote:

On Mon, Jan 25, 2021 at 3:25 PM Martin KaFai Lau [off-list ref] wrote:

quoted

On Mon, Jan 25, 2021 at 09:26:40AM -0800, Stanislav Fomichev wrote:

quoted

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.

quoted

[ ... ]

quoted

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?

Ah, right.  Then there is no need to set enforce_attach_type_range at all.
"enforce_attach_type_range = tnum_range(0, 3);" can be removed.

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?

It is because, from the very beginning, BPF_PROG_TYPE_CGROUP_SKB did not
enforce the attach_type in bpf_prog_attach_check_attach_type().