[PATCH v2 bpf-next 0/4] Introduce bpf_ct_set_nat_info kfunc helper

[PATCH v2 bpf-next 0/4] Introduce bpf_ct_set_nat_info kfunc helper

[Lorenzo Bianconi]

Introduce bpf_ct_set_nat_info kfunc helper in order to set source and
destination nat addresses/ports in a new allocated ct entry not inserted
in the connection tracking table yet.
Introduce support for per-parameter trusted args.

Changes since v1:
- enable CONFIG_NF_NAT in tools/testing/selftests/bpf/config

Kumar Kartikeya Dwivedi (2):
  bpf: Add support for per-parameter trusted args
  selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

Lorenzo Bianconi (2):
  net: netfilter: add bpf_ct_set_nat_info kfunc helper
  selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

 Documentation/bpf/kfuncs.rst                  | 18 +++++++
 kernel/bpf/btf.c                              | 39 ++++++++++-----
 net/bpf/test_run.c                            |  9 +++-
 net/netfilter/nf_conntrack_bpf.c              | 49 ++++++++++++++++++-
 tools/testing/selftests/bpf/config            |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 +
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 +++++++++-
 tools/testing/selftests/bpf/verifier/calls.c  | 38 +++++++++++---
 8 files changed, 157 insertions(+), 25 deletions(-)

-- 
2.37.3

[PATCH v2 bpf-next 1/4] bpf: Add support for per-parameter trusted args

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Similar to how we detect mem, size pairs in kfunc, teach verifier to
treat __ref suffix on argument name to imply that it must be a trusted
arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
but limited to the specific parameter. This is required to ensure that
kfunc that operate on some object only work on acquired pointers and not
normal PTR_TO_BTF_ID with same type which can be obtained by pointer
walking. Release functions need not specify such suffix on release
arguments as they are already expected to receive one referenced
argument.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
 kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
 net/bpf/test_run.c           |  9 +++++++--
 3 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index 781731749e55..a9d77d12fd0c 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type
 of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
 pointer.
 
+2.2.2 __ref Annotation
+----------------------
+
+This annotation is used to indicate that the argument is trusted, i.e. it will
+be a pointer from an acquire function (defined later), and its offset will be
+zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but
+only on the parameter it is applied to. An example is shown below::
+
+        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
+        {
+        ...
+        }
+
+Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
+cannot be used on pointers obtained using pointer walking. This ensures that
+caller always calls this kfunc on a task whose lifetime is guaranteed for the
+duration of the call.
+
 .. _BPF_kfunc_nodef:
 
 2.3 Using an existing kernel function
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 903719b89238..7e273f949ee8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6140,18 +6140,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
 	return true;
 }
 
-static bool is_kfunc_arg_mem_size(const struct btf *btf,
-				  const struct btf_param *arg,
-				  const struct bpf_reg_state *reg)
+static bool btf_param_match_suffix(const struct btf *btf,
+				   const struct btf_param *arg,
+				   const char *suffix)
 {
-	int len, sfx_len = sizeof("__sz") - 1;
-	const struct btf_type *t;
+	int len, sfx_len = strlen(suffix);
 	const char *param_name;
 
-	t = btf_type_skip_modifiers(btf, arg->type, NULL);
-	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
-		return false;
-
 	/* In the future, this can be ported to use BTF tagging */
 	param_name = btf_name_by_offset(btf, arg->name_off);
 	if (str_is_empty(param_name))
@@ -6160,10 +6155,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
 	if (len < sfx_len)
 		return false;
 	param_name += len - sfx_len;
-	if (strncmp(param_name, "__sz", sfx_len))
+	return !strncmp(param_name, suffix, sfx_len);
+}
+
+static bool is_kfunc_arg_ref(const struct btf *btf,
+			     const struct btf_param *arg)
+{
+	return btf_param_match_suffix(btf, arg, "__ref");
+}
+
+static bool is_kfunc_arg_mem_size(const struct btf *btf,
+				  const struct btf_param *arg,
+				  const struct bpf_reg_state *reg)
+{
+	const struct btf_type *t;
+
+	t = btf_type_skip_modifiers(btf, arg->type, NULL);
+	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
 		return false;
 
-	return true;
+	return btf_param_match_suffix(btf, arg, "__sz");
 }
 
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
@@ -6173,7 +6184,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    u32 kfunc_flags)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
-	bool rel = false, kptr_get = false, trusted_arg = false;
+	bool rel = false, kptr_get = false, kf_trusted_args = false;
 	bool sleepable = false;
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;
@@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Only kfunc can be release func */
 		rel = kfunc_flags & KF_RELEASE;
 		kptr_get = kfunc_flags & KF_KPTR_GET;
-		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
+		kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
 		sleepable = kfunc_flags & KF_SLEEPABLE;
 	}
 
@@ -6222,6 +6233,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		enum bpf_arg_type arg_type = ARG_DONTCARE;
 		u32 regno = i + 1;
 		struct bpf_reg_state *reg = &regs[regno];
+		bool trusted_arg = false;
 
 		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
 		if (btf_type_is_scalar(t)) {
@@ -6240,6 +6252,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Check if argument must be a referenced pointer, args + i has
 		 * been verified to be a pointer (after skipping modifiers).
 		 */
+		trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
 		if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
 			bpf_log(log, "R%d must be referenced\n", regno);
 			return -EINVAL;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 25d8ecf105aa..b735accf8750 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
 {
 }
 
-noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
+noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
 {
 }
 
@@ -722,7 +726,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
 BTF_SET8_END(test_sk_check_kfunc_ids)
 
-- 
2.37.3

[PATCH v2 bpf-next 2/4] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Extend the existing test for KF_TRUSTED_ARGS by also checking whether
the same happens when a __ref suffix is present in argument name of a
kfunc.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index 3fb4f69b1962..891fcda50d9d 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -219,7 +219,7 @@
 	.errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
 },
 {
-	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (KF_TRUSTED_ARGS)",
 	.insns = {
 	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
 	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
@@ -227,10 +227,30 @@
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
 	BPF_EXIT_INSN(),
-	BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
-	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
-	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, 16),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_trusted", 7 },
+	},
+	.result_unpriv = REJECT,
+	.result = REJECT,
+	.errstr = "R1 must be referenced",
+},
+{
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (__ref)",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_MOV64_IMM(BPF_REG_0, 0),
 	BPF_EXIT_INSN(),
@@ -238,8 +258,7 @@
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.fixup_kfunc_btf_id = {
 		{ "bpf_kfunc_call_test_acquire", 3 },
-		{ "bpf_kfunc_call_test_ref", 8 },
-		{ "bpf_kfunc_call_test_ref", 10 },
+		{ "bpf_kfunc_call_test_ref", 7 },
 	},
 	.result_unpriv = REJECT,
 	.result = REJECT,
@@ -259,14 +278,17 @@
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_MOV64_IMM(BPF_REG_0, 0),
 	BPF_EXIT_INSN(),
 	},
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.fixup_kfunc_btf_id = {
 		{ "bpf_kfunc_call_test_acquire", 3 },
-		{ "bpf_kfunc_call_test_ref", 8 },
-		{ "bpf_kfunc_call_test_release", 10 },
+		{ "bpf_kfunc_call_test_trusted", 8 },
+		{ "bpf_kfunc_call_test_ref", 10 },
+		{ "bpf_kfunc_call_test_release", 12 },
 	},
 	.result_unpriv = REJECT,
 	.result = ACCEPT,
-- 
2.37.3

[PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Lorenzo Bianconi]

Introduce bpf_ct_set_nat_info kfunc helper in order to set source and
destination nat addresses/ports in a new allocated ct entry not inserted
in the connection tracking table yet.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 net/netfilter/nf_conntrack_bpf.c | 49 +++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index 1cd87b28c9b0..85b8c7ee00af 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -14,6 +14,7 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_bpf.h>
 #include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_nat.h>
 
 /* bpf_ct_opts - Options for CT lookup helpers
  *
@@ -134,7 +135,6 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
 
 	memset(&ct->proto, 0, sizeof(ct->proto));
 	__nf_ct_set_timeout(ct, timeout * HZ);
-	ct->status |= IPS_CONFIRMED;
 
 out:
 	if (opts->netns_id >= 0)
@@ -339,6 +339,7 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
 	struct nf_conn *nfct = (struct nf_conn *)nfct_i;
 	int err;
 
+	nfct->status |= IPS_CONFIRMED;
 	err = nf_conntrack_hash_check_insert(nfct);
 	if (err < 0) {
 		nf_conntrack_free(nfct);
@@ -424,6 +425,51 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
 	return nf_ct_change_status_common(nfct, status);
 }
 
+/* bpf_ct_set_nat_info - Set source or destination nat address
+ *
+ * Set source or destination nat address of the newly allocated
+ * nf_conn before insertion. This must be invoked for referenced
+ * PTR_TO_BTF_ID to nf_conn___init.
+ *
+ * Parameters:
+ * @nfct	- Pointer to referenced nf_conn object, obtained using
+ *		  bpf_xdp_ct_alloc or bpf_skb_ct_alloc.
+ * @addr	- Nat source/destination address
+ * @port	- Nat source/destination port
+ * @manip	- NF_NAT_MANIP_SRC or NF_NAT_MANIP_DST
+ */
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+			union nf_inet_addr *addr, __be16 *port,
+			enum nf_nat_manip_type manip)
+{
+#if ((IS_MODULE(CONFIG_NF_NAT) && IS_MODULE(CONFIG_NF_CONNTRACK)) || \
+     IS_BUILTIN(CONFIG_NF_NAT))
+	struct nf_conn *ct = (struct nf_conn *)nfct__ref;
+	u16 proto = nf_ct_l3num(ct);
+	struct nf_nat_range2 range;
+
+	if (proto != NFPROTO_IPV4 && proto != NFPROTO_IPV6)
+		return -EINVAL;
+
+	if (!addr)
+		return -EINVAL;
+
+	memset(&range, 0, sizeof(struct nf_nat_range2));
+	range.flags = NF_NAT_RANGE_MAP_IPS;
+	range.min_addr = *addr;
+	range.max_addr = *addr;
+	if (port) {
+		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		range.min_proto.all = *port;
+		range.max_proto.all = *port;
+	}
+
+	return nf_nat_setup_info(ct, &range, manip) == NF_DROP ? -ENOMEM : 0;
+#else
+	return -EOPNOTSUPP;
+#endif
+}
+
 __diag_pop()
 
 BTF_SET8_START(nf_ct_kfunc_set)
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)
 
 static const struct btf_kfunc_id_set nf_conntrack_kfunc_set = {
-- 
2.37.3

[PATCH v2 bpf-next 4/4] selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

[Lorenzo Bianconi]

Introduce self-tests for bpf_ct_set_nat_info kfunc used to set the
source or destination nat addresses/ports.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/testing/selftests/bpf/config            |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 ++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 ++++++++++++++++++-
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 3fc46f9cfb22..8ce48f7213cb 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -57,6 +57,7 @@ CONFIG_NF_CONNTRACK=y
 CONFIG_NF_CONNTRACK_MARK=y
 CONFIG_NF_DEFRAG_IPV4=y
 CONFIG_NF_DEFRAG_IPV6=y
+CONFIG_NF_NAT=y
 CONFIG_RC_CORE=y
 CONFIG_SECURITY=y
 CONFIG_SECURITYFS=y
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
index 544bf90ac2a7..f16913f8fca2 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
@@ -115,6 +115,8 @@ static void test_bpf_nf_ct(int mode)
 	ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
 	ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
 	ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
+	ASSERT_EQ(skel->data->test_snat_addr, 0, "Test for source natting");
+	ASSERT_EQ(skel->data->test_dnat_addr, 0, "Test for destination natting");
 end:
 	if (srv_client_fd != -1)
 		close(srv_client_fd);
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
index 2722441850cc..3f441595098b 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
@@ -23,6 +23,8 @@ int test_insert_entry = -EAFNOSUPPORT;
 int test_succ_lookup = -ENOENT;
 u32 test_delta_timeout = 0;
 u32 test_status = 0;
+int test_snat_addr = -EINVAL;
+int test_dnat_addr = -EINVAL;
 __be32 saddr = 0;
 __be16 sport = 0;
 __be32 daddr = 0;
@@ -53,6 +55,8 @@ void bpf_ct_set_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_set_status(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_status(struct nf_conn *, u32) __ksym;
+int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
+			__be16 *port, enum nf_nat_manip_type) __ksym;
 
 static __always_inline void
 nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
@@ -140,10 +144,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
 	ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
 		      sizeof(opts_def));
 	if (ct) {
+		__be16 sport = bpf_get_prandom_u32();
+		__be16 dport = bpf_get_prandom_u32();
+		union nf_inet_addr saddr = {};
+		union nf_inet_addr daddr = {};
 		struct nf_conn *ct_ins;
 
 		bpf_ct_set_timeout(ct, 10000);
-		bpf_ct_set_status(ct, IPS_CONFIRMED);
+		/* snat */
+		saddr.ip = bpf_get_prandom_u32();
+		bpf_ct_set_nat_info(ct, &saddr, &sport, NF_NAT_MANIP_SRC);
+		/* dnat */
+		daddr.ip = bpf_get_prandom_u32();
+		bpf_ct_set_nat_info(ct, &daddr, &dport, NF_NAT_MANIP_DST);
 
 		ct_ins = bpf_ct_insert_entry(ct);
 		if (ct_ins) {
@@ -152,6 +165,17 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
 			ct_lk = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4),
 					  &opts_def, sizeof(opts_def));
 			if (ct_lk) {
+				struct nf_conntrack_tuple *tuple;
+
+				/* check snat and dnat addresses */
+				tuple = &ct_lk->tuplehash[IP_CT_DIR_REPLY].tuple;
+				if (tuple->dst.u3.ip == saddr.ip &&
+				    tuple->dst.u.all == sport)
+					test_snat_addr = 0;
+				if (tuple->src.u3.ip == daddr.ip &&
+				    tuple->src.u.all == dport)
+					test_dnat_addr = 0;
+
 				/* update ct entry timeout */
 				bpf_ct_change_timeout(ct_lk, 10000);
 				test_delta_timeout = ct_lk->timeout - bpf_jiffies64();
-- 
2.37.3

Re: [PATCH v2 bpf-next 1/4] bpf: Add support for per-parameter trusted args

[Song Liu]

On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> Similar to how we detect mem, size pairs in kfunc, teach verifier to
> treat __ref suffix on argument name to imply that it must be a trusted
> arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> but limited to the specific parameter. This is required to ensure that
> kfunc that operate on some object only work on acquired pointers and not
> normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> walking. Release functions need not specify such suffix on release
> arguments as they are already expected to receive one referenced
> argument.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

LGTM:

Acked-by: Song Liu <song@kernel.org>

PS: IIUC, we use "refcounted" and "trusted" for the same concept. Shall we
just use __trusted? Pardon me if this has been discussed before.

Thanks,
Song

> ---
>  Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
>  kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
>  net/bpf/test_run.c           |  9 +++++++--
>  3 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index 781731749e55..a9d77d12fd0c 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type
>  of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
>  pointer.
>
> +2.2.2 __ref Annotation
> +----------------------
> +
> +This annotation is used to indicate that the argument is trusted, i.e. it will
> +be a pointer from an acquire function (defined later), and its offset will be
> +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but
> +only on the parameter it is applied to. An example is shown below::
> +
> +        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
> +        {
> +        ...
> +        }
> +
> +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
> +cannot be used on pointers obtained using pointer walking. This ensures that
> +caller always calls this kfunc on a task whose lifetime is guaranteed for the
> +duration of the call.
> +
>  .. _BPF_kfunc_nodef:
>
>  2.3 Using an existing kernel function
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 903719b89238..7e273f949ee8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6140,18 +6140,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
>         return true;
>  }
>
> -static bool is_kfunc_arg_mem_size(const struct btf *btf,
> -                                 const struct btf_param *arg,
> -                                 const struct bpf_reg_state *reg)
> +static bool btf_param_match_suffix(const struct btf *btf,
> +                                  const struct btf_param *arg,
> +                                  const char *suffix)
>  {
> -       int len, sfx_len = sizeof("__sz") - 1;
> -       const struct btf_type *t;
> +       int len, sfx_len = strlen(suffix);
>         const char *param_name;
>
> -       t = btf_type_skip_modifiers(btf, arg->type, NULL);
> -       if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> -               return false;
> -
>         /* In the future, this can be ported to use BTF tagging */
>         param_name = btf_name_by_offset(btf, arg->name_off);
>         if (str_is_empty(param_name))
> @@ -6160,10 +6155,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
>         if (len < sfx_len)
>                 return false;
>         param_name += len - sfx_len;
> -       if (strncmp(param_name, "__sz", sfx_len))
> +       return !strncmp(param_name, suffix, sfx_len);
> +}
> +
> +static bool is_kfunc_arg_ref(const struct btf *btf,
> +                            const struct btf_param *arg)
> +{
> +       return btf_param_match_suffix(btf, arg, "__ref");
> +}
> +
> +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> +                                 const struct btf_param *arg,
> +                                 const struct bpf_reg_state *reg)
> +{
> +       const struct btf_type *t;
> +
> +       t = btf_type_skip_modifiers(btf, arg->type, NULL);
> +       if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
>                 return false;
>
> -       return true;
> +       return btf_param_match_suffix(btf, arg, "__sz");
>  }
>
>  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> @@ -6173,7 +6184,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                                     u32 kfunc_flags)
>  {
>         enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> -       bool rel = false, kptr_get = false, trusted_arg = false;
> +       bool rel = false, kptr_get = false, kf_trusted_args = false;
>         bool sleepable = false;
>         struct bpf_verifier_log *log = &env->log;
>         u32 i, nargs, ref_id, ref_obj_id = 0;
> @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                 /* Only kfunc can be release func */
>                 rel = kfunc_flags & KF_RELEASE;
>                 kptr_get = kfunc_flags & KF_KPTR_GET;
> -               trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> +               kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
>                 sleepable = kfunc_flags & KF_SLEEPABLE;
>         }
>
> @@ -6222,6 +6233,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                 enum bpf_arg_type arg_type = ARG_DONTCARE;
>                 u32 regno = i + 1;
>                 struct bpf_reg_state *reg = &regs[regno];
> +               bool trusted_arg = false;
>
>                 t = btf_type_skip_modifiers(btf, args[i].type, NULL);
>                 if (btf_type_is_scalar(t)) {
> @@ -6240,6 +6252,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                 /* Check if argument must be a referenced pointer, args + i has
>                  * been verified to be a pointer (after skipping modifiers).
>                  */
> +               trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
>                 if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
>                         bpf_log(log, "R%d must be referenced\n", regno);
>                         return -EINVAL;
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 25d8ecf105aa..b735accf8750 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
>  {
>  }
>
> -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
> +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
> +{
> +}
> +
> +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
>  {
>  }
>
> @@ -722,7 +726,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
> -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
>  BTF_SET8_END(test_sk_check_kfunc_ids)
>
> --
> 2.37.3
>

Re: [PATCH v2 bpf-next 2/4] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

[Song Liu]

On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> Extend the existing test for KF_TRUSTED_ARGS by also checking whether
> the same happens when a __ref suffix is present in argument name of a
> kfunc.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Acked-by: Song Liu <song@kernel.org>

> ---
>  tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++-----
>  1 file changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
> index 3fb4f69b1962..891fcda50d9d 100644
> --- a/tools/testing/selftests/bpf/verifier/calls.c
> +++ b/tools/testing/selftests/bpf/verifier/calls.c
> @@ -219,7 +219,7 @@
>         .errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
>  },
>  {
> -       "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
> +       "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (KF_TRUSTED_ARGS)",
>         .insns = {
>         BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
>         BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
> @@ -227,10 +227,30 @@
>         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
>         BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
>         BPF_EXIT_INSN(),
> -       BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
> -       BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> +       BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
>         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
> -       BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, 16),
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_EXIT_INSN(),
> +       },
> +       .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> +       .fixup_kfunc_btf_id = {
> +               { "bpf_kfunc_call_test_acquire", 3 },
> +               { "bpf_kfunc_call_test_trusted", 7 },
> +       },
> +       .result_unpriv = REJECT,
> +       .result = REJECT,
> +       .errstr = "R1 must be referenced",
> +},
> +{
> +       "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (__ref)",
> +       .insns = {
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
> +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
> +       BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
> +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
> +       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
> +       BPF_EXIT_INSN(),
> +       BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
>         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
>         BPF_MOV64_IMM(BPF_REG_0, 0),
>         BPF_EXIT_INSN(),
> @@ -238,8 +258,7 @@
>         .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>         .fixup_kfunc_btf_id = {
>                 { "bpf_kfunc_call_test_acquire", 3 },
> -               { "bpf_kfunc_call_test_ref", 8 },
> -               { "bpf_kfunc_call_test_ref", 10 },
> +               { "bpf_kfunc_call_test_ref", 7 },
>         },
>         .result_unpriv = REJECT,
>         .result = REJECT,
> @@ -259,14 +278,17 @@
>         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
>         BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
>         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
>         BPF_MOV64_IMM(BPF_REG_0, 0),
>         BPF_EXIT_INSN(),
>         },
>         .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>         .fixup_kfunc_btf_id = {
>                 { "bpf_kfunc_call_test_acquire", 3 },
> -               { "bpf_kfunc_call_test_ref", 8 },
> -               { "bpf_kfunc_call_test_release", 10 },
> +               { "bpf_kfunc_call_test_trusted", 8 },
> +               { "bpf_kfunc_call_test_ref", 10 },
> +               { "bpf_kfunc_call_test_release", 12 },
>         },
>         .result_unpriv = REJECT,
>         .result = ACCEPT,
> --
> 2.37.3
>

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Song Liu]

On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> Introduce bpf_ct_set_nat_info kfunc helper in order to set source and
> destination nat addresses/ports in a new allocated ct entry not inserted
> in the connection tracking table yet.
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  net/netfilter/nf_conntrack_bpf.c | 49 +++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index 1cd87b28c9b0..85b8c7ee00af 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
> @@ -14,6 +14,7 @@
>  #include <net/netfilter/nf_conntrack.h>
>  #include <net/netfilter/nf_conntrack_bpf.h>
>  #include <net/netfilter/nf_conntrack_core.h>
> +#include <net/netfilter/nf_nat.h>
>
>  /* bpf_ct_opts - Options for CT lookup helpers
>   *
> @@ -134,7 +135,6 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
>
>         memset(&ct->proto, 0, sizeof(ct->proto));
>         __nf_ct_set_timeout(ct, timeout * HZ);
> -       ct->status |= IPS_CONFIRMED;
>
>  out:
>         if (opts->netns_id >= 0)
> @@ -339,6 +339,7 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
>         struct nf_conn *nfct = (struct nf_conn *)nfct_i;
>         int err;
>
> +       nfct->status |= IPS_CONFIRMED;
>         err = nf_conntrack_hash_check_insert(nfct);
>         if (err < 0) {
>                 nf_conntrack_free(nfct);
> @@ -424,6 +425,51 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
>         return nf_ct_change_status_common(nfct, status);
>  }

Why do we need the above two changes in this patch?

Thanks,
Song

Re: [PATCH v2 bpf-next 4/4] selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

[Song Liu]

On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> Introduce self-tests for bpf_ct_set_nat_info kfunc used to set the
> source or destination nat addresses/ports.
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  tools/testing/selftests/bpf/config            |  1 +
>  .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 ++
>  .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 ++++++++++++++++++-
>  3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index 3fc46f9cfb22..8ce48f7213cb 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config
> @@ -57,6 +57,7 @@ CONFIG_NF_CONNTRACK=y
>  CONFIG_NF_CONNTRACK_MARK=y
>  CONFIG_NF_DEFRAG_IPV4=y
>  CONFIG_NF_DEFRAG_IPV6=y
> +CONFIG_NF_NAT=y
>  CONFIG_RC_CORE=y
>  CONFIG_SECURITY=y
>  CONFIG_SECURITYFS=y
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> index 544bf90ac2a7..f16913f8fca2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> @@ -115,6 +115,8 @@ static void test_bpf_nf_ct(int mode)
>         ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
>         ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
>         ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
> +       ASSERT_EQ(skel->data->test_snat_addr, 0, "Test for source natting");
> +       ASSERT_EQ(skel->data->test_dnat_addr, 0, "Test for destination natting");
>  end:
>         if (srv_client_fd != -1)
>                 close(srv_client_fd);
> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> index 2722441850cc..3f441595098b 100644
> --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> @@ -23,6 +23,8 @@ int test_insert_entry = -EAFNOSUPPORT;
>  int test_succ_lookup = -ENOENT;
>  u32 test_delta_timeout = 0;
>  u32 test_status = 0;
> +int test_snat_addr = -EINVAL;
> +int test_dnat_addr = -EINVAL;
>  __be32 saddr = 0;
>  __be16 sport = 0;
>  __be32 daddr = 0;
> @@ -53,6 +55,8 @@ void bpf_ct_set_timeout(struct nf_conn *, u32) __ksym;
>  int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym;
>  int bpf_ct_set_status(struct nf_conn *, u32) __ksym;
>  int bpf_ct_change_status(struct nf_conn *, u32) __ksym;
> +int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
> +                       __be16 *port, enum nf_nat_manip_type) __ksym;
>
>  static __always_inline void
>  nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
> @@ -140,10 +144,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
>         ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
>                       sizeof(opts_def));
>         if (ct) {
> +               __be16 sport = bpf_get_prandom_u32();
> +               __be16 dport = bpf_get_prandom_u32();
> +               union nf_inet_addr saddr = {};
> +               union nf_inet_addr daddr = {};
>                 struct nf_conn *ct_ins;
>
>                 bpf_ct_set_timeout(ct, 10000);
> -               bpf_ct_set_status(ct, IPS_CONFIRMED);

So this is paired with the IPS_CONFIRMED change in 3/4?

Thanks,
Song

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Alexei Starovoitov]

On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> +                       union nf_inet_addr *addr, __be16 *port,
> +                       enum nf_nat_manip_type manip)
> +{
...
> @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
>  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
>  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
>  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
>  BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to
change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted".
They're not refcounted per verifier definition,
but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean
"this helper can assume that all pointers can be safely
accessed without worrying about lifetime".

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Kumar Kartikeya Dwivedi]

On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > +                       union nf_inet_addr *addr, __be16 *port,
> > +                       enum nf_nat_manip_type manip)
> > +{
> ...
> > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> >  BTF_SET8_END(nf_ct_kfunc_set)
>
> Instead of __ref and patch 1 and 2 it would be better to
> change the meaning of "trusted_args".
> In this case "addr" and "port" are just as "trusted".
> They're not refcounted per verifier definition,
> but they need to be "trusted" by the helper.
> At the end the "trusted_args" flags would mean
> "this helper can assume that all pointers can be safely
> accessed without worrying about lifetime".

So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?

But suppose in the future you have a type that has scalars only.

struct foo { int a; int b; ... };
Just data, and this is acquired from a kfunc and released using another kfunc.
Now with this new definition you are proposing, verifier ends up
allowing PTR_TO_MEM to also be passed to such helpers for the struct
foo *.

I guess even reg->ref_obj_id check is not enough, user may also pass
PTR_TO_MEM | MEM_ALLOC which can be refcounted.

It would be easy to forget such subtle details later.

What we want to actually force here is 'please give me refcounted
PTR_TO_BTF_ID to foo'.
So maybe KF_TRUSTED_ARGS should change meaning to what you described,
and then a __btf tag should force PTR_TO_BTF_ID.
KF_TRUSTED_ARGS then just forces reg->ref_obj_id for PTR_TO_BTF_ID.

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Alexei Starovoitov]

On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > > +                       union nf_inet_addr *addr, __be16 *port,
> > > +                       enum nf_nat_manip_type manip)
> > > +{
> > ...
> > > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> > >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> > >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> > >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> > >  BTF_SET8_END(nf_ct_kfunc_set)
> >
> > Instead of __ref and patch 1 and 2 it would be better to
> > change the meaning of "trusted_args".
> > In this case "addr" and "port" are just as "trusted".
> > They're not refcounted per verifier definition,
> > but they need to be "trusted" by the helper.
> > At the end the "trusted_args" flags would mean
> > "this helper can assume that all pointers can be safely
> > accessed without worrying about lifetime".
>
> So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?
>
> But suppose in the future you have a type that has scalars only.
>
> struct foo { int a; int b; ... };
> Just data, and this is acquired from a kfunc and released using another kfunc.
> Now with this new definition you are proposing, verifier ends up
> allowing PTR_TO_MEM to also be passed to such helpers for the struct
> foo *.
>
> I guess even reg->ref_obj_id check is not enough, user may also pass
> PTR_TO_MEM | MEM_ALLOC which can be refcounted.
>
> It would be easy to forget such subtle details later.

It may add headaches to the verifier side, but here we have to
think from pov of other subsystems that add kfuncs.
They shouldn't need to know the verifier details.
The internals will change anyway.
Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc
will use to indicate that the function assumes valid pointers.
How the verifier recognizes them is irrelevant from kfunc pov.
People that write bpf progs are not that much different from
people that write kfuncs that bpf progs use.
Both should be easy to write.

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Kumar Kartikeya Dwivedi]

On Wed, 7 Sept 2022 at 07:15, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > > > +                       union nf_inet_addr *addr, __be16 *port,
> > > > +                       enum nf_nat_manip_type manip)
> > > > +{
> > > ...
> > > > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> > > >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> > > >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> > > >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > > > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> > > >  BTF_SET8_END(nf_ct_kfunc_set)
> > >
> > > Instead of __ref and patch 1 and 2 it would be better to
> > > change the meaning of "trusted_args".
> > > In this case "addr" and "port" are just as "trusted".
> > > They're not refcounted per verifier definition,
> > > but they need to be "trusted" by the helper.
> > > At the end the "trusted_args" flags would mean
> > > "this helper can assume that all pointers can be safely
> > > accessed without worrying about lifetime".
> >
> > So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?
> >
> > But suppose in the future you have a type that has scalars only.
> >
> > struct foo { int a; int b; ... };
> > Just data, and this is acquired from a kfunc and released using another kfunc.
> > Now with this new definition you are proposing, verifier ends up
> > allowing PTR_TO_MEM to also be passed to such helpers for the struct
> > foo *.
> >
> > I guess even reg->ref_obj_id check is not enough, user may also pass
> > PTR_TO_MEM | MEM_ALLOC which can be refcounted.
> >
> > It would be easy to forget such subtle details later.
>
> It may add headaches to the verifier side, but here we have to
> think from pov of other subsystems that add kfuncs.
> They shouldn't need to know the verifier details.
> The internals will change anyway.

Ok, I'll go with making it work for all args for this case.

> Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc
> will use to indicate that the function assumes valid pointers.
> How the verifier recognizes them is irrelevant from kfunc pov.
> People that write bpf progs are not that much different from
> people that write kfuncs that bpf progs use.
> Both should be easy to write.

That is a worthy goal, but it can't become the default unless we
somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to
be valid, valid-looking-but-uaf pointer, NULL all at the same time
depending on how it was obtained. Currently all helpers, even stable
ones, are broken in this regard. Similarly recently added
cgroup_rstat_flush etc. kfuncs are equally unsafe.

All stable helpers taking PTR_TO_BTF_ID are not even checking for at
least NULL, even though it's right there in bpf.h.
   592         /* PTR_TO_BTF_ID points to a kernel struct that does not need
   593          * to be null checked by the BPF program. This does not imply the
   594          * pointer is _not_ null and in practice this can
easily be a null
   595          * pointer when reading pointer chains. The assumption is program
which just proves how confusing it is right now. And "fixing" that by
adding a NULL check doesn't fix it completely, since it can also be a
seemingly valid looking but freed pointer.

My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID
pointers from loads from PTR_TO_CTX of tracing progs into this
definition of 'trusted', but not those obtained from walking them. It
works for iterator arguments also.

We could limit these restrictions only to kfuncs instead of stable helpers.

It might be possible to instead just whitelist the function BTF IDs as
well, even saying pointers from walks are also safe in this context
for the kfuncs allowed there, or we work on annotating the safe cases
using BTF tags.

There are some problems currently (GCC not supporting BTF tags yet, is
argument really trusted in fexit program in 'xyz_free' function), but
overall it seems like a better state than status quo. It might also
finally push GCC to begin supporting BTF tags.

Mapping of a set of btf_ids can be done to a specific kfunc hook
(instead of current program type), so you are basically composing a
kfunc hook out of a set of btf_ids instead of program type. It
represents a safe context to call those kfuncs in.

It is impossible to know otherwise what case is safe to call a kfunc
for and what is not statically - short of also allowing the unsafe
cases.

Then the kfuncs work on refcounted pointers, and also unrefcounted
ones for known safe cases (basically where the lifetime is guaranteed
by bpf program caller). For arguments it works by default. The only
extra work is annotating things inside structures.
Might not even need that extra annotation in many cases, since kernel
already has __rcu etc. which we can start recognizing like __user to
complain in non-sleepable programs (e.g. without explicit RCU section
which may be added in the future).

Then just flip KF_TRUSTED_ARGS by default, and people have to opt into
'unsafe' instead to make it work for some edge cases, with a big fat
warning for the user of that kfunc.

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

> On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > Introduce bpf_ct_set_nat_info kfunc helper in order to set source and
> > destination nat addresses/ports in a new allocated ct entry not inserted
> > in the connection tracking table yet.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  net/netfilter/nf_conntrack_bpf.c | 49 +++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> > index 1cd87b28c9b0..85b8c7ee00af 100644
> > --- a/net/netfilter/nf_conntrack_bpf.c
> > +++ b/net/netfilter/nf_conntrack_bpf.c
> > @@ -14,6 +14,7 @@
> >  #include <net/netfilter/nf_conntrack.h>
> >  #include <net/netfilter/nf_conntrack_bpf.h>
> >  #include <net/netfilter/nf_conntrack_core.h>
> > +#include <net/netfilter/nf_nat.h>
> >
> >  /* bpf_ct_opts - Options for CT lookup helpers
> >   *
> > @@ -134,7 +135,6 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
> >
> >         memset(&ct->proto, 0, sizeof(ct->proto));
> >         __nf_ct_set_timeout(ct, timeout * HZ);
> > -       ct->status |= IPS_CONFIRMED;
> >
> >  out:
> >         if (opts->netns_id >= 0)
> > @@ -339,6 +339,7 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
> >         struct nf_conn *nfct = (struct nf_conn *)nfct_i;
> >         int err;
> >
> > +       nfct->status |= IPS_CONFIRMED;
> >         err = nf_conntrack_hash_check_insert(nfct);
> >         if (err < 0) {
> >                 nf_conntrack_free(nfct);
> > @@ -424,6 +425,51 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
> >         return nf_ct_change_status_common(nfct, status);
> >  }
> 
> Why do we need the above two changes in this patch?

nf_nat_setup_info() does not really add the nat info in the connection tracking
entry if it is already confirmed (it just returns NF_ACCEPT). I moved
IPS_CONFIRMED in bpf_ct_insert_entry() since we can run bpf_ct_set_nat_info()
just if the entry has not inserted in the table yet.

Regards,
Lorenzo

> 
> Thanks,
> Song

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 bpf-next 4/4] selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3991 bytes --]

> On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > Introduce self-tests for bpf_ct_set_nat_info kfunc used to set the
> > source or destination nat addresses/ports.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/config            |  1 +
> >  .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 ++
> >  .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 ++++++++++++++++++-
> >  3 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > index 3fc46f9cfb22..8ce48f7213cb 100644
> > --- a/tools/testing/selftests/bpf/config
> > +++ b/tools/testing/selftests/bpf/config
> > @@ -57,6 +57,7 @@ CONFIG_NF_CONNTRACK=y
> >  CONFIG_NF_CONNTRACK_MARK=y
> >  CONFIG_NF_DEFRAG_IPV4=y
> >  CONFIG_NF_DEFRAG_IPV6=y
> > +CONFIG_NF_NAT=y
> >  CONFIG_RC_CORE=y
> >  CONFIG_SECURITY=y
> >  CONFIG_SECURITYFS=y
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> > index 544bf90ac2a7..f16913f8fca2 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> > @@ -115,6 +115,8 @@ static void test_bpf_nf_ct(int mode)
> >         ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
> >         ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
> >         ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
> > +       ASSERT_EQ(skel->data->test_snat_addr, 0, "Test for source natting");
> > +       ASSERT_EQ(skel->data->test_dnat_addr, 0, "Test for destination natting");
> >  end:
> >         if (srv_client_fd != -1)
> >                 close(srv_client_fd);
> > diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> > index 2722441850cc..3f441595098b 100644
> > --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> > +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> > @@ -23,6 +23,8 @@ int test_insert_entry = -EAFNOSUPPORT;
> >  int test_succ_lookup = -ENOENT;
> >  u32 test_delta_timeout = 0;
> >  u32 test_status = 0;
> > +int test_snat_addr = -EINVAL;
> > +int test_dnat_addr = -EINVAL;
> >  __be32 saddr = 0;
> >  __be16 sport = 0;
> >  __be32 daddr = 0;
> > @@ -53,6 +55,8 @@ void bpf_ct_set_timeout(struct nf_conn *, u32) __ksym;
> >  int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym;
> >  int bpf_ct_set_status(struct nf_conn *, u32) __ksym;
> >  int bpf_ct_change_status(struct nf_conn *, u32) __ksym;
> > +int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
> > +                       __be16 *port, enum nf_nat_manip_type) __ksym;
> >
> >  static __always_inline void
> >  nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
> > @@ -140,10 +144,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
> >         ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
> >                       sizeof(opts_def));
> >         if (ct) {
> > +               __be16 sport = bpf_get_prandom_u32();
> > +               __be16 dport = bpf_get_prandom_u32();
> > +               union nf_inet_addr saddr = {};
> > +               union nf_inet_addr daddr = {};
> >                 struct nf_conn *ct_ins;
> >
> >                 bpf_ct_set_timeout(ct, 10000);
> > -               bpf_ct_set_status(ct, IPS_CONFIRMED);
> 
> So this is paired with the IPS_CONFIRMED change in 3/4?

we actually do not need it since it is already done during entry allocation (or
insertion).
Looking again at the code I spotted a bug since we do not really check the value
configured with bpf_ct_change_status(ct_lk, IPS_SEEN_REPLY). I will post a fix.

Regards,
Lorenzo

> 
> Thanks,
> Song

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Alexei Starovoitov]

On Tue, Sep 6, 2022 at 10:52 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Wed, 7 Sept 2022 at 07:15, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > > > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > > > > +                       union nf_inet_addr *addr, __be16 *port,
> > > > > +                       enum nf_nat_manip_type manip)
> > > > > +{
> > > > ...
> > > > > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> > > > >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> > > > >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> > > > >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > > > > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> > > > >  BTF_SET8_END(nf_ct_kfunc_set)
> > > >
> > > > Instead of __ref and patch 1 and 2 it would be better to
> > > > change the meaning of "trusted_args".
> > > > In this case "addr" and "port" are just as "trusted".
> > > > They're not refcounted per verifier definition,
> > > > but they need to be "trusted" by the helper.
> > > > At the end the "trusted_args" flags would mean
> > > > "this helper can assume that all pointers can be safely
> > > > accessed without worrying about lifetime".
> > >
> > > So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?
> > >
> > > But suppose in the future you have a type that has scalars only.
> > >
> > > struct foo { int a; int b; ... };
> > > Just data, and this is acquired from a kfunc and released using another kfunc.
> > > Now with this new definition you are proposing, verifier ends up
> > > allowing PTR_TO_MEM to also be passed to such helpers for the struct
> > > foo *.
> > >
> > > I guess even reg->ref_obj_id check is not enough, user may also pass
> > > PTR_TO_MEM | MEM_ALLOC which can be refcounted.
> > >
> > > It would be easy to forget such subtle details later.
> >
> > It may add headaches to the verifier side, but here we have to
> > think from pov of other subsystems that add kfuncs.
> > They shouldn't need to know the verifier details.
> > The internals will change anyway.
>
> Ok, I'll go with making it work for all args for this case.
>
> > Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc
> > will use to indicate that the function assumes valid pointers.
> > How the verifier recognizes them is irrelevant from kfunc pov.
> > People that write bpf progs are not that much different from
> > people that write kfuncs that bpf progs use.
> > Both should be easy to write.
>
> That is a worthy goal, but it can't become the default unless we
> somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to
> be valid, valid-looking-but-uaf pointer, NULL all at the same time
> depending on how it was obtained. Currently all helpers, even stable
> ones, are broken in this regard. Similarly recently added
> cgroup_rstat_flush etc. kfuncs are equally unsafe.
>
> All stable helpers taking PTR_TO_BTF_ID are not even checking for at
> least NULL, even though it's right there in bpf.h.
>    592         /* PTR_TO_BTF_ID points to a kernel struct that does not need
>    593          * to be null checked by the BPF program. This does not imply the
>    594          * pointer is _not_ null and in practice this can
> easily be a null
>    595          * pointer when reading pointer chains. The assumption is program
> which just proves how confusing it is right now. And "fixing" that by
> adding a NULL check doesn't fix it completely, since it can also be a
> seemingly valid looking but freed pointer.
>
> My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID
> pointers from loads from PTR_TO_CTX of tracing progs into this
> definition of 'trusted', but not those obtained from walking them. It
> works for iterator arguments also.
>
> We could limit these restrictions only to kfuncs instead of stable helpers.
>
> It might be possible to instead just whitelist the function BTF IDs as
> well, even saying pointers from walks are also safe in this context
> for the kfuncs allowed there, or we work on annotating the safe cases
> using BTF tags.
>
> There are some problems currently (GCC not supporting BTF tags yet, is
> argument really trusted in fexit program in 'xyz_free' function), but
> overall it seems like a better state than status quo. It might also
> finally push GCC to begin supporting BTF tags.
>
> Mapping of a set of btf_ids can be done to a specific kfunc hook
> (instead of current program type), so you are basically composing a
> kfunc hook out of a set of btf_ids instead of program type. It
> represents a safe context to call those kfuncs in.
>
> It is impossible to know otherwise what case is safe to call a kfunc
> for and what is not statically - short of also allowing the unsafe
> cases.
>
> Then the kfuncs work on refcounted pointers, and also unrefcounted
> ones for known safe cases (basically where the lifetime is guaranteed
> by bpf program caller). For arguments it works by default. The only
> extra work is annotating things inside structures.
> Might not even need that extra annotation in many cases, since kernel
> already has __rcu etc. which we can start recognizing like __user to
> complain in non-sleepable programs (e.g. without explicit RCU section
> which may be added in the future).
>
> Then just flip KF_TRUSTED_ARGS by default, and people have to opt into
> 'unsafe' instead to make it work for some edge cases, with a big fat
> warning for the user of that kfunc.

With few minor nits, that I don't want to get into right now,
all of the above makes sense. It can be a plan of record.
But all that will be done later.
The immediate first step I'm proposing is
to extend the definition of KF_TRUSTED_ARGS to include this
particular use case of:
union nf_inet_addr *addr, __be16 *port,

Those won't be PTR_TO_BTF_ID so above plan doesn't affect this case.
They're PTR_TO_MEM (if I'm reading the selftest in the next patch
correctly) and we can relax:
                if (is_kfunc && trusted_arg && !reg->ref_obj_id) {

Just minimal amount of verifier work to enable this specific
bpf_ct_set_nat_info kfunc.

I think that's user friendlier approach than __ref suffix
which forces kfunc writers to understand all of the above
verifier details (valid-looking-but-uaf, null-but-not-null, etc).

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Kumar Kartikeya Dwivedi]

On Wed, 7 Sept 2022 at 19:33, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 6, 2022 at 10:52 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Wed, 7 Sept 2022 at 07:15, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > > > > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > > > > > +                       union nf_inet_addr *addr, __be16 *port,
> > > > > > +                       enum nf_nat_manip_type manip)
> > > > > > +{
> > > > > ...
> > > > > > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> > > > > >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> > > > > >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> > > > > >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > > > > > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> > > > > >  BTF_SET8_END(nf_ct_kfunc_set)
> > > > >
> > > > > Instead of __ref and patch 1 and 2 it would be better to
> > > > > change the meaning of "trusted_args".
> > > > > In this case "addr" and "port" are just as "trusted".
> > > > > They're not refcounted per verifier definition,
> > > > > but they need to be "trusted" by the helper.
> > > > > At the end the "trusted_args" flags would mean
> > > > > "this helper can assume that all pointers can be safely
> > > > > accessed without worrying about lifetime".
> > > >
> > > > So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?
> > > >
> > > > But suppose in the future you have a type that has scalars only.
> > > >
> > > > struct foo { int a; int b; ... };
> > > > Just data, and this is acquired from a kfunc and released using another kfunc.
> > > > Now with this new definition you are proposing, verifier ends up
> > > > allowing PTR_TO_MEM to also be passed to such helpers for the struct
> > > > foo *.
> > > >
> > > > I guess even reg->ref_obj_id check is not enough, user may also pass
> > > > PTR_TO_MEM | MEM_ALLOC which can be refcounted.
> > > >
> > > > It would be easy to forget such subtle details later.
> > >
> > > It may add headaches to the verifier side, but here we have to
> > > think from pov of other subsystems that add kfuncs.
> > > They shouldn't need to know the verifier details.
> > > The internals will change anyway.
> >
> > Ok, I'll go with making it work for all args for this case.
> >
> > > Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc
> > > will use to indicate that the function assumes valid pointers.
> > > How the verifier recognizes them is irrelevant from kfunc pov.
> > > People that write bpf progs are not that much different from
> > > people that write kfuncs that bpf progs use.
> > > Both should be easy to write.
> >
> > That is a worthy goal, but it can't become the default unless we
> > somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to
> > be valid, valid-looking-but-uaf pointer, NULL all at the same time
> > depending on how it was obtained. Currently all helpers, even stable
> > ones, are broken in this regard. Similarly recently added
> > cgroup_rstat_flush etc. kfuncs are equally unsafe.
> >
> > All stable helpers taking PTR_TO_BTF_ID are not even checking for at
> > least NULL, even though it's right there in bpf.h.
> >    592         /* PTR_TO_BTF_ID points to a kernel struct that does not need
> >    593          * to be null checked by the BPF program. This does not imply the
> >    594          * pointer is _not_ null and in practice this can
> > easily be a null
> >    595          * pointer when reading pointer chains. The assumption is program
> > which just proves how confusing it is right now. And "fixing" that by
> > adding a NULL check doesn't fix it completely, since it can also be a
> > seemingly valid looking but freed pointer.
> >
> > My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID
> > pointers from loads from PTR_TO_CTX of tracing progs into this
> > definition of 'trusted', but not those obtained from walking them. It
> > works for iterator arguments also.
> >
> > We could limit these restrictions only to kfuncs instead of stable helpers.
> >
> > It might be possible to instead just whitelist the function BTF IDs as
> > well, even saying pointers from walks are also safe in this context
> > for the kfuncs allowed there, or we work on annotating the safe cases
> > using BTF tags.
> >
> > There are some problems currently (GCC not supporting BTF tags yet, is
> > argument really trusted in fexit program in 'xyz_free' function), but
> > overall it seems like a better state than status quo. It might also
> > finally push GCC to begin supporting BTF tags.
> >
> > Mapping of a set of btf_ids can be done to a specific kfunc hook
> > (instead of current program type), so you are basically composing a
> > kfunc hook out of a set of btf_ids instead of program type. It
> > represents a safe context to call those kfuncs in.
> >
> > It is impossible to know otherwise what case is safe to call a kfunc
> > for and what is not statically - short of also allowing the unsafe
> > cases.
> >
> > Then the kfuncs work on refcounted pointers, and also unrefcounted
> > ones for known safe cases (basically where the lifetime is guaranteed
> > by bpf program caller). For arguments it works by default. The only
> > extra work is annotating things inside structures.
> > Might not even need that extra annotation in many cases, since kernel
> > already has __rcu etc. which we can start recognizing like __user to
> > complain in non-sleepable programs (e.g. without explicit RCU section
> > which may be added in the future).
> >
> > Then just flip KF_TRUSTED_ARGS by default, and people have to opt into
> > 'unsafe' instead to make it work for some edge cases, with a big fat
> > warning for the user of that kfunc.
>
> With few minor nits, that I don't want to get into right now,
> all of the above makes sense. It can be a plan of record.
> But all that will be done later.
> The immediate first step I'm proposing is
> to extend the definition of KF_TRUSTED_ARGS to include this
> particular use case of:
> union nf_inet_addr *addr, __be16 *port,
>
> Those won't be PTR_TO_BTF_ID so above plan doesn't affect this case.
> They're PTR_TO_MEM (if I'm reading the selftest in the next patch
> correctly) and we can relax:
>                 if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
>
> Just minimal amount of verifier work to enable this specific
> bpf_ct_set_nat_info kfunc.
>
> I think that's user friendlier approach than __ref suffix
> which forces kfunc writers to understand all of the above
> verifier details (valid-looking-but-uaf, null-but-not-null, etc).

I agree, it seems better from a UX standpoint. I'll relax the
trusted_args check for non-PTR_TO_BTF_ID, and send the patch to flip
it to default as an RFC later.