[PATCH bpf-next v3 0/4] bpf: verifier: use target program's type for access verifications

[PATCH bpf-next v3 0/4] bpf: verifier: use target program's type for access verifications

[Udip Pant]

This patch series adds changes in verifier to make decisions such as granting
of read / write access or enforcement of return code status based on
the program type of the target program while using dynamic program
extension (of type BPF_PROG_TYPE_EXT).

The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
placeholder for those, we need this extended check for those extended
programs to actually work with proper access, while using this option.

Patch #1 includes changes in the verifier.
Patch #2 adds selftests to verify write access on a packet for a valid 
extension program type
Patch #3 adds selftests to verify proper check for the return code
Patch #4 adds selftests to ensure access permissions and restrictions 
for some map types such sockmap.

Changelogs:
  v2 -> v3:
    * more comprehensive resolution of the program type in the verifier
      based on the target program (and not just for the packet access)
    * selftests for checking return code and map access
    * Also moved this patch to 'bpf-next' from 'bpf' tree
  v1 -> v2:
    * extraction of the logic to resolve prog type into a separate method
    * selftests to check for packet access for a valid freplace prog

Udip Pant (4):
  bpf: verifier: use target program's type for access verifications
  selftests/bpf: add test for freplace program with write access
  selftests/bpf: test for checking return code for the extended prog
  selftests/bpf: test for map update access from within EXT programs

 kernel/bpf/verifier.c                         | 32 ++++++---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 68 +++++++++++++++++++
 .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 ++++++++
 .../bpf/progs/freplace_attach_probe.c         | 40 +++++++++++
 .../bpf/progs/freplace_cls_redirect.c         | 34 ++++++++++
 .../bpf/progs/freplace_connect_v4_prog.c      | 19 ++++++
 .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++
 7 files changed, 229 insertions(+), 11 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_attach_probe.c
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_connect_v4_prog.c

-- 
2.24.1

[PATCH bpf-next v3 1/4] bpf: verifier: use target program's type for access verifications

[Udip Pant]

This patch adds changes in verifier to make decisions such as granting
of read / write access or enforcement of return code status based on
the program type of the target program while using dynamic program
extension (of type BPF_PROG_TYPE_EXT).

The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
placeholder for those, we need this extended check for those extended
programs to actually work with proper access, while using this option.

Specifically, it introduces following changes:
- may_access_direct_pkt_data:
    allow access to packet data based on the target prog
- check_return_code:
    enforce return code based on the target prog
    (currently, this check is skipped for EXT program)
- check_ld_abs:
    check for 'may_access_skb' based on the target prog
- check_map_prog_compatibility:
    enforce the map compatibility check based on the target prog
- may_update_sockmap:
    allow sockmap update based on the target prog

Some other occurrences of prog->type is left as it without replacing
with the 'resolved' type:
- do_check_common() and check_attach_btf_id():
    already have specific logic to handle the EXT prog type
- jit_subprogs() and bpf_check():
    Not changed for jit compilation or while inferring env->ops

Next few patches in this series include selftests for some of these cases.

Signed-off-by: Udip Pant <udippant@fb.com>
---
 kernel/bpf/verifier.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 38748794518e..95c715508034 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2625,11 +2625,19 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
 
 #define MAX_PACKET_OFF 0xffff
 
+static enum bpf_prog_type resolve_prog_type(struct bpf_prog *prog)
+{
+	return prog->aux->linked_prog ? prog->aux->linked_prog->type
+				      : prog->type;
+}
+
 static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
 				       const struct bpf_call_arg_meta *meta,
 				       enum bpf_access_type t)
 {
-	switch (env->prog->type) {
+	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
+
+	switch (prog_type) {
 	/* Program types only with direct read access go here! */
 	case BPF_PROG_TYPE_LWT_IN:
 	case BPF_PROG_TYPE_LWT_OUT:
@@ -4181,7 +4189,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
 {
 	enum bpf_attach_type eatype = env->prog->expected_attach_type;
-	enum bpf_prog_type type = env->prog->type;
+	enum bpf_prog_type type = resolve_prog_type(env->prog);
 
 	if (func_id != BPF_FUNC_map_update_elem)
 		return false;
@@ -7366,7 +7374,7 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
 	u8 mode = BPF_MODE(insn->code);
 	int i, err;
 
-	if (!may_access_skb(env->prog->type)) {
+	if (!may_access_skb(resolve_prog_type(env->prog))) {
 		verbose(env, "BPF_LD_[ABS|IND] instructions not allowed for this program type\n");
 		return -EINVAL;
 	}
@@ -7454,11 +7462,12 @@ static int check_return_code(struct bpf_verifier_env *env)
 	const struct bpf_prog *prog = env->prog;
 	struct bpf_reg_state *reg;
 	struct tnum range = tnum_range(0, 1);
+	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
 	int err;
 
 	/* LSM and struct_ops func-ptr's return type could be "void" */
-	if ((env->prog->type == BPF_PROG_TYPE_STRUCT_OPS ||
-	     env->prog->type == BPF_PROG_TYPE_LSM) &&
+	if ((prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
+	     prog_type == BPF_PROG_TYPE_LSM) &&
 	    !prog->aux->attach_func_proto->type)
 		return 0;
 
@@ -7477,7 +7486,7 @@ static int check_return_code(struct bpf_verifier_env *env)
 		return -EACCES;
 	}
 
-	switch (env->prog->type) {
+	switch (prog_type) {
 	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
 		if (env->prog->expected_attach_type == BPF_CGROUP_UDP4_RECVMSG ||
 		    env->prog->expected_attach_type == BPF_CGROUP_UDP6_RECVMSG ||
@@ -9233,6 +9242,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 					struct bpf_prog *prog)
 
 {
+	enum bpf_prog_type prog_type = resolve_prog_type(prog);
 	/*
 	 * Validate that trace type programs use preallocated hash maps.
 	 *
@@ -9250,8 +9260,8 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 	 * now, but warnings are emitted so developers are made aware of
 	 * the unsafety and can fix their programs before this is enforced.
 	 */
-	if (is_tracing_prog_type(prog->type) && !is_preallocated_map(map)) {
-		if (prog->type == BPF_PROG_TYPE_PERF_EVENT) {
+	if (is_tracing_prog_type(prog_type) && !is_preallocated_map(map)) {
+		if (prog_type == BPF_PROG_TYPE_PERF_EVENT) {
 			verbose(env, "perf_event programs can only use preallocated hash map\n");
 			return -EINVAL;
 		}
@@ -9263,8 +9273,8 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 		verbose(env, "trace type programs with run-time allocated hash maps are unsafe. Switch to preallocated hash maps.\n");
 	}
 
-	if ((is_tracing_prog_type(prog->type) ||
-	     prog->type == BPF_PROG_TYPE_SOCKET_FILTER) &&
+	if ((is_tracing_prog_type(prog_type) ||
+	     prog_type == BPF_PROG_TYPE_SOCKET_FILTER) &&
 	    map_value_has_spin_lock(map)) {
 		verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
 		return -EINVAL;
@@ -9976,7 +9986,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 				insn->code = BPF_LDX | BPF_PROBE_MEM |
 					BPF_SIZE((insn)->code);
 				env->prog->aux->num_exentries++;
-			} else if (env->prog->type != BPF_PROG_TYPE_STRUCT_OPS) {
+			} else if (resolve_prog_type(env->prog) != BPF_PROG_TYPE_STRUCT_OPS) {
 				verbose(env, "Writes through BTF pointers are not allowed\n");
 				return -EINVAL;
 			}
-- 
2.24.1

[PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace program with write access

[Udip Pant]

This adds a selftest that tests the behavior when a freplace target program
attempts to make a write access on a packet. The expectation is that the read or write
access is granted based on the program type of the linked program and
not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).

This test fails without the associated patch on the verifier.

Signed-off-by: Udip Pant <udippant@fb.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  1 +
 .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 +++++++++++++++++++
 .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 197d0d217b56..7c7168963d52 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -123,6 +123,7 @@ static void test_func_replace(void)
 		"freplace/get_skb_len",
 		"freplace/get_skb_ifindex",
 		"freplace/get_constant",
+		"freplace/test_pkt_write_access_subprog",
 	};
 	test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
 				  "./test_pkt_access.o",
diff --git a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
index 98e1efe14549..49a84a3a2306 100644
--- a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2019 Facebook */
 #include <linux/stddef.h>
+#include <linux/if_ether.h>
 #include <linux/ipv6.h>
 #include <linux/bpf.h>
+#include <linux/tcp.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 #include <bpf/bpf_tracing.h>
@@ -151,4 +153,29 @@ int new_get_constant(long val)
 	test_get_constant = 1;
 	return test_get_constant; /* original get_constant() returns val - 122 */
 }
+
+__u64 test_pkt_write_access_subprog = 0;
+SEC("freplace/test_pkt_write_access_subprog")
+int new_test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
+{
+
+	void *data = (void *)(long)skb->data;
+	void *data_end = (void *)(long)skb->data_end;
+	struct tcphdr *tcp;
+
+	if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
+		return -1;
+
+	tcp = data + off;
+	if (tcp + 1 > data_end)
+		return -1;
+
+	/* make modifications to the packet data */
+	tcp->check++;
+	tcp->syn = 0;
+
+	test_pkt_write_access_subprog = 1;
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_pkt_access.c b/tools/testing/selftests/bpf/progs/test_pkt_access.c
index e72eba4a93d2..852051064507 100644
--- a/tools/testing/selftests/bpf/progs/test_pkt_access.c
+++ b/tools/testing/selftests/bpf/progs/test_pkt_access.c
@@ -79,6 +79,24 @@ int get_skb_ifindex(int val, struct __sk_buff *skb, int var)
 	return skb->ifindex * val * var;
 }
 
+__attribute__ ((noinline))
+int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
+{
+	void *data = (void *)(long)skb->data;
+	void *data_end = (void *)(long)skb->data_end;
+	struct tcphdr *tcp = NULL;
+
+	if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
+		return -1;
+
+	tcp = data + off;
+	if (tcp + 1 > data_end)
+		return -1;
+	/* make modification to the packet data */
+	tcp->check++;
+	return 0;
+}
+
 SEC("classifier/test_pkt_access")
 int test_pkt_access(struct __sk_buff *skb)
 {
@@ -117,6 +135,8 @@ int test_pkt_access(struct __sk_buff *skb)
 	if (test_pkt_access_subprog3(3, skb) != skb->len * 3 * skb->ifindex)
 		return TC_ACT_SHOT;
 	if (tcp) {
+		if (test_pkt_write_access_subprog(skb, (void *)tcp - data))
+			return TC_ACT_SHOT;
 		if (((void *)(tcp) + 20) > data_end || proto != 6)
 			return TC_ACT_SHOT;
 		barrier(); /* to force ordering of checks */
-- 
2.24.1

[PATCH bpf-next v3 3/4] selftests/bpf: test for checking return code for the extended prog

[Udip Pant]

This adds test to enforce same check for the return code for the extended prog
as it is enforced for the target program. It asserts failure for a
return code, which is permitted without the patch in this series, while
it is restricted after the application of this patch.

Signed-off-by: Udip Pant <udippant@fb.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 40 +++++++++++++++++++
 .../bpf/progs/freplace_connect_v4_prog.c      | 19 +++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_connect_v4_prog.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 7c7168963d52..d295ca9bbf96 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -142,10 +142,50 @@ static void test_func_replace_verify(void)
 				  prog_name, false);
 }
 
+static void test_func_replace_return_code(void)
+{
+	/*
+	 * standalone test that asserts failure to load freplace prog
+	 * because of invalid return code.
+	 */
+	struct bpf_object *obj = NULL, *pkt_obj;
+	int err, pkt_fd;
+	__u32 duration = 0;
+	const char *target_obj_file = "./connect4_prog.o";
+	const char *obj_file = "./freplace_connect_v4_prog.o";
+
+	err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
+			    &pkt_obj, &pkt_fd);
+	/* the target prog should load fine */
+	if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
+		  target_obj_file, err, errno))
+		return;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
+			    .attach_prog_fd = pkt_fd,
+			   );
+
+	obj = bpf_object__open_file(obj_file, &opts);
+	if (CHECK(IS_ERR_OR_NULL(obj), "obj_open",
+		  "failed to open %s: %ld\n", obj_file,
+		  PTR_ERR(obj)))
+		goto close_prog;
+
+	/* It should fail to load the program */
+	err = bpf_object__load(obj);
+	if (CHECK(!err, "bpf_obj_load should fail", "err %d\n", err))
+		goto close_prog;
+
+close_prog:
+	if (!IS_ERR_OR_NULL(obj))
+		bpf_object__close(obj);
+	bpf_object__close(pkt_obj);
+}
+
 void test_fexit_bpf2bpf(void)
 {
 	test_target_no_callees();
 	test_target_yes_callees();
 	test_func_replace();
 	test_func_replace_verify();
+	test_func_replace_return_code();
 }
diff --git a/tools/testing/selftests/bpf/progs/freplace_connect_v4_prog.c b/tools/testing/selftests/bpf/progs/freplace_connect_v4_prog.c
new file mode 100644
index 000000000000..544e5ac90461
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_connect_v4_prog.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Facebook
+
+#include <linux/stddef.h>
+#include <linux/ipv6.h>
+#include <linux/bpf.h>
+#include <linux/in.h>
+#include <sys/socket.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+SEC("freplace/connect_v4_prog")
+int new_connect_v4_prog(struct bpf_sock_addr *ctx)
+{
+	// return value thats in invalid range
+	return 255;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.24.1

[PATCH bpf-next v3 4/4] selftests/bpf: test for map update access from within EXT programs

[Udip Pant]

This adds further tests to ensure access permissions and restrictions
are applied properly for some map types such as sock-map.
It also adds another negative tests to assert static functions cannot be
replaced. In the 'unreliable' mode it still fails with error 'tracing progs
cannot use bpf_spin_lock yet' with the change in the verifier

Signed-off-by: Udip Pant <udippant@fb.com>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 33 +++++++++++++--
 .../bpf/progs/freplace_attach_probe.c         | 40 +++++++++++++++++++
 .../bpf/progs/freplace_cls_redirect.c         | 34 ++++++++++++++++
 3 files changed, 104 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_attach_probe.c
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_cls_redirect.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index d295ca9bbf96..a550dab9ba7a 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -142,7 +142,20 @@ static void test_func_replace_verify(void)
 				  prog_name, false);
 }
 
-static void test_func_replace_return_code(void)
+static void test_func_sockmap_update(void)
+{
+	const char *prog_name[] = {
+		"freplace/cls_redirect",
+	};
+	test_fexit_bpf2bpf_common("./freplace_cls_redirect.o",
+				  "./test_cls_redirect.o",
+				  ARRAY_SIZE(prog_name),
+				  prog_name, false);
+}
+
+static void test_obj_load_failure_common(const char *obj_file,
+					  const char *target_obj_file)
+
 {
 	/*
 	 * standalone test that asserts failure to load freplace prog
@@ -151,8 +164,6 @@ static void test_func_replace_return_code(void)
 	struct bpf_object *obj = NULL, *pkt_obj;
 	int err, pkt_fd;
 	__u32 duration = 0;
-	const char *target_obj_file = "./connect4_prog.o";
-	const char *obj_file = "./freplace_connect_v4_prog.o";
 
 	err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
 			    &pkt_obj, &pkt_fd);
@@ -181,11 +192,27 @@ static void test_func_replace_return_code(void)
 	bpf_object__close(pkt_obj);
 }
 
+static void test_func_replace_return_code(void)
+{
+	/* test invalid return code in the replaced program */
+	test_obj_load_failure_common("./freplace_connect_v4_prog.o",
+				     "./connect4_prog.o");
+}
+
+static void test_func_map_prog_compatibility(void)
+{
+	/* test with spin lock map value in the replaced program */
+	test_obj_load_failure_common("./freplace_attach_probe.o",
+				     "./test_attach_probe.o");
+}
+
 void test_fexit_bpf2bpf(void)
 {
 	test_target_no_callees();
 	test_target_yes_callees();
 	test_func_replace();
 	test_func_replace_verify();
+	test_func_sockmap_update();
 	test_func_replace_return_code();
+	test_func_map_prog_compatibility();
 }
diff --git a/tools/testing/selftests/bpf/progs/freplace_attach_probe.c b/tools/testing/selftests/bpf/progs/freplace_attach_probe.c
new file mode 100644
index 000000000000..bb2a77c5b62b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_attach_probe.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Facebook
+
+#include <linux/ptrace.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define VAR_NUM 2
+
+struct hmap_elem {
+	struct bpf_spin_lock lock;
+	int var[VAR_NUM];
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, struct hmap_elem);
+} hash_map SEC(".maps");
+
+SEC("freplace/handle_kprobe")
+int new_handle_kprobe(struct pt_regs *ctx)
+{
+	struct hmap_elem zero = {}, *val;
+	int key = 0;
+
+	val = bpf_map_lookup_elem(&hash_map, &key);
+	if (!val)
+		return 1;
+	/* spin_lock in hash map */
+	bpf_spin_lock(&val->lock);
+	val->var[0] = 99;
+	bpf_spin_unlock(&val->lock);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c b/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
new file mode 100644
index 000000000000..68a5a9db928a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Facebook
+
+#include <linux/stddef.h>
+#include <linux/bpf.h>
+#include <linux/pkt_cls.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_helpers.h>
+
+struct bpf_map_def SEC("maps") sock_map = {
+	.type = BPF_MAP_TYPE_SOCKMAP,
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.max_entries = 2,
+};
+
+SEC("freplace/cls_redirect")
+int freplace_cls_redirect_test(struct __sk_buff *skb)
+{
+	int ret = 0;
+	const int zero = 0;
+	struct bpf_sock *sk;
+
+	sk = bpf_map_lookup_elem(&sock_map, &zero);
+	if (!sk)
+		return TC_ACT_SHOT;
+
+	ret = bpf_map_update_elem(&sock_map, &zero, sk, 0);
+	bpf_sk_release(sk);
+
+	return ret == 0 ? TC_ACT_OK : TC_ACT_SHOT;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.24.1

Re: [PATCH bpf-next v3 0/4] bpf: verifier: use target program's type for access verifications

[Yonghong Song]

On 8/25/20 4:19 PM, Udip Pant wrote:
> This patch series adds changes in verifier to make decisions such as granting
> of read / write access or enforcement of return code status based on
> the program type of the target program while using dynamic program
> extension (of type BPF_PROG_TYPE_EXT).
> 
> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> placeholder for those, we need this extended check for those extended
> programs to actually work with proper access, while using this option.
> 
> Patch #1 includes changes in the verifier.
> Patch #2 adds selftests to verify write access on a packet for a valid
> extension program type
> Patch #3 adds selftests to verify proper check for the return code
> Patch #4 adds selftests to ensure access permissions and restrictions
> for some map types such sockmap.
> 
> Changelogs:
>    v2 -> v3:
>      * more comprehensive resolution of the program type in the verifier
>        based on the target program (and not just for the packet access)
>      * selftests for checking return code and map access
>      * Also moved this patch to 'bpf-next' from 'bpf' tree
>    v1 -> v2:
>      * extraction of the logic to resolve prog type into a separate method
>      * selftests to check for packet access for a valid freplace prog
> 
> Udip Pant (4):
>    bpf: verifier: use target program's type for access verifications
>    selftests/bpf: add test for freplace program with write access
>    selftests/bpf: test for checking return code for the extended prog
>    selftests/bpf: test for map update access from within EXT programs
> 
>   kernel/bpf/verifier.c                         | 32 ++++++---
>   .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 68 +++++++++++++++++++
>   .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 ++++++++
>   .../bpf/progs/freplace_attach_probe.c         | 40 +++++++++++
>   .../bpf/progs/freplace_cls_redirect.c         | 34 ++++++++++
>   .../bpf/progs/freplace_connect_v4_prog.c      | 19 ++++++
>   .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++
>   7 files changed, 229 insertions(+), 11 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/freplace_attach_probe.c
>   create mode 100644 tools/testing/selftests/bpf/progs/freplace_cls_redirect.c
>   create mode 100644 tools/testing/selftests/bpf/progs/freplace_connect_v4_prog.c

Thanks. LGTM. Ack for the whole series.
Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next v3 0/4] bpf: verifier: use target program's type for access verifications

[Toke Høiland-Jørgensen]

Udip Pant <udippant@fb.com> writes:

> This patch series adds changes in verifier to make decisions such as granting
> of read / write access or enforcement of return code status based on
> the program type of the target program while using dynamic program
> extension (of type BPF_PROG_TYPE_EXT).
>
> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> placeholder for those, we need this extended check for those extended
> programs to actually work with proper access, while using this option.
>
> Patch #1 includes changes in the verifier.
> Patch #2 adds selftests to verify write access on a packet for a valid 
> extension program type
> Patch #3 adds selftests to verify proper check for the return code
> Patch #4 adds selftests to ensure access permissions and restrictions 
> for some map types such sockmap.

Thanks for fixing this!

For the series:
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH bpf-next v3 0/4] bpf: verifier: use target program's type for access verifications

[Alexei Starovoitov]

On Tue, Aug 25, 2020 at 04:19:59PM -0700, Udip Pant wrote:
> This patch series adds changes in verifier to make decisions such as granting
> of read / write access or enforcement of return code status based on
> the program type of the target program while using dynamic program
> extension (of type BPF_PROG_TYPE_EXT).
> 
> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> placeholder for those, we need this extended check for those extended
> programs to actually work with proper access, while using this option.
> 
> Patch #1 includes changes in the verifier.
> Patch #2 adds selftests to verify write access on a packet for a valid 
> extension program type
> Patch #3 adds selftests to verify proper check for the return code
> Patch #4 adds selftests to ensure access permissions and restrictions 
> for some map types such sockmap.
> 
> Changelogs:
>   v2 -> v3:
>     * more comprehensive resolution of the program type in the verifier
>       based on the target program (and not just for the packet access)
>     * selftests for checking return code and map access
>     * Also moved this patch to 'bpf-next' from 'bpf' tree

Applied. Thanks

Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace program with write access

[Andrii Nakryiko]

On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <udippant@fb.com> wrote:
>
> This adds a selftest that tests the behavior when a freplace target program
> attempts to make a write access on a packet. The expectation is that the read or write
> access is granted based on the program type of the linked program and
> not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).
>
> This test fails without the associated patch on the verifier.
>
> Signed-off-by: Udip Pant <udippant@fb.com>
> ---
>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  1 +
>  .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 +++++++++++++++++++
>  .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++++++++++
>  3 files changed, 48 insertions(+)
>

[...]

> +__attribute__ ((noinline))
> +int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
> +{
> +       void *data = (void *)(long)skb->data;
> +       void *data_end = (void *)(long)skb->data_end;
> +       struct tcphdr *tcp = NULL;
> +
> +       if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
> +               return -1;
> +
> +       tcp = data + off;
> +       if (tcp + 1 > data_end)
> +               return -1;
> +       /* make modification to the packet data */
> +       tcp->check++;

Just FYI for all BPF contributors. This change makes test_pkt_access
BPF program to fail on kernel 5.5, which (the kernel) we use as part
libbpf CI testing. test_pkt_access.o in turn makes few different
selftests (see [0] for details) to fail on 5.5 (because
test_pkt_access is used as one of BPF objects loaded as part of those
selftests). This is ok, I'm blacklisting (at least temporarily) those
tests, but I wanted to bring up this issue, as it did happen before
and will keep happening in the future and will constantly decrease
test coverage for older kernels that libbpf CI performs.

I propose that when we introduce new features (like new fields in a
BPF program's context or something along those lines) and want to test
them, we should lean towards creating new tests, not modify existing
ones. This will allow all already working selftests to keep working
for older kernels. Does this sound reasonable as an approach?

As for this particular breakage, I'd appreciate someone taking a look
at the problem and checking if it's some new feature that's not
present in 5.5 or just Clang/verifier interactions (32-bit pointer
arithmetic, is this a new issue?). If it's something fixable, it would
be nice to fix and restore 5.5 tests. Thanks!

  [0] https://travis-ci.com/github/libbpf/libbpf/jobs/378226438

Verifier complains about:

; if (test_pkt_write_access_subprog(skb, (void *)tcp - data))

57: (79) r1 = *(u64 *)(r10 -8)

58: (bc) w2 = w1

59: (1c) w2 -= w9

R2 32-bit pointer arithmetic prohibited

processed 198 insns (limit 1000000) max_states_per_insn 1 total_states
8 peak_states 8 mark_read 7


> +       return 0;
> +}
> +
>  SEC("classifier/test_pkt_access")
>  int test_pkt_access(struct __sk_buff *skb)
>  {
> @@ -117,6 +135,8 @@ int test_pkt_access(struct __sk_buff *skb)
>         if (test_pkt_access_subprog3(3, skb) != skb->len * 3 * skb->ifindex)
>                 return TC_ACT_SHOT;
>         if (tcp) {
> +               if (test_pkt_write_access_subprog(skb, (void *)tcp - data))
> +                       return TC_ACT_SHOT;
>                 if (((void *)(tcp) + 20) > data_end || proto != 6)
>                         return TC_ACT_SHOT;
>                 barrier(); /* to force ordering of checks */
> --
> 2.24.1
>

Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace program with write access

[Udip Pant]

On 8/26/20, 11:05 PM, "Andrii Nakryiko" <andrii.nakryiko@gmail.com> wrote:

On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <udippant@fb.com> wrote:
>>
>> This adds a selftest that tests the behavior when a freplace target program
>> attempts to make a write access on a packet. The expectation is that the read or write
>> access is granted based on the program type of the linked program and
>> not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).
>>
>> This test fails without the associated patch on the verifier.
>>
>> Signed-off-by: Udip Pant <udippant@fb.com>
>> ---
>>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  1 +
>>  .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 +++++++++++++++++++
>>  .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++++++++++
>>  3 files changed, 48 insertions(+)
>>
>
>[...]
>
>> +__attribute__ ((noinline))
>> +int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
>> +{
>> +       void *data = (void *)(long)skb->data;
>> +       void *data_end = (void *)(long)skb->data_end;
>> +       struct tcphdr *tcp = NULL;
>> +
>> +       if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
>> +               return -1;
>> +
>> +       tcp = data + off;
>> +       if (tcp + 1 > data_end)
>> +               return -1;
>> +       /* make modification to the packet data */
>> +       tcp->check++;
>
> Just FYI for all BPF contributors. This change makes test_pkt_access
> BPF program to fail on kernel 5.5, which (the kernel) we use as part
> libbpf CI testing. test_pkt_access.o in turn makes few different
> selftests (see [0] for details) to fail on 5.5 (because
> test_pkt_access is used as one of BPF objects loaded as part of those
> selftests). This is ok, I'm blacklisting (at least temporarily) those
> tests, but I wanted to bring up this issue, as it did happen before
> and will keep happening in the future and will constantly decrease
> test coverage for older kernels that libbpf CI performs.
>
> I propose that when we introduce new features (like new fields in a
> BPF program's context or something along those lines) and want to test
> them, we should lean towards creating new tests, not modify existing
> ones. This will allow all already working selftests to keep working
> for older kernels. Does this sound reasonable as an approach?
>
> As for this particular breakage, I'd appreciate someone taking a look
> at the problem and checking if it's some new feature that's not
> present in 5.5 or just Clang/verifier interactions (32-bit pointer
> arithmetic, is this a new issue?). If it's something fixable, it would
> be nice to fix and restore 5.5 tests. Thanks!
>
>   [0] https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.com_github_libbpf_libbpf_jobs_378226438&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=EICQWIMWWJPbefL5Ad4oTQ&m=ezWtkpxlrj19nFAuBX5LswTLCVR3zCtVY7MBlIsm7bA&s=zzYFn7QWYsp3BDOxYP95S4yKr2kqndGw1w6zHoNaRdU&e= 
>
> Verifier complains about:
>
> ; if (test_pkt_write_access_subprog(skb, (void *)tcp - data))

Not sure about this specific issue with 32-bit pointer arithmetic. But I can try a workaround to fix this test by passing only the skb (and deriving the tcp-header inside the method).