[2/2] selftests/bpf: add negative tests for relaxed fixed offset constraint on trusted pointer arguments

Commit Message

Matt Bobrowski June 17, 2024, 1:44 p.m. UTC

Adding some new negative selftests which are responsible for asserting
that the new relaxed fixed offset constraints applicable to BPF
helpers and kfuncs taking trusted pointer arguments are enforced
correctly by the BPF verifier.

The BPF programs contained within the new negative selftests are
mainly responsible for triggering the various branches and checks
performed within the check_release_arg_reg_off() helper.

Signed-off-by: Matt Bobrowski <[e-mail]>
---
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 .../bpf/progs/verifier_arg_reg_off_reject.c   | 154 ++++++++++++++++++
 2 files changed, 156 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_arg_reg_off_reject.c

Comments

Kumar Kartikeya Dwivedi June 19, 2024, 9:02 a.m. UTC | #1

On Mon, 17 Jun 2024 at 15:44, Matt Bobrowski <[e-mail]> wrote:
>
> Adding some new negative selftests which are responsible for asserting
> that the new relaxed fixed offset constraints applicable to BPF
> helpers and kfuncs taking trusted pointer arguments are enforced
> correctly by the BPF verifier.
>
> The BPF programs contained within the new negative selftests are
> mainly responsible for triggering the various branches and checks
> performed within the check_release_arg_reg_off() helper.
>
> Signed-off-by: Matt Bobrowski <[e-mail]>
> ---

With the stuff below addressed, please add:
Acked-by: Kumar Kartikeya Dwivedi <[e-mail]>

I believe sockmap map_update and dynptr ones are also providing
positive coverage (given previous versions made them fail), so I think
focusing on negative cases is good enough here. I guess bpf_sk_release
is also covered by existing ones.

Still it'd be good to add a few more cases if you find any, as Eduard
suggested, just to make sure we're not missing anything.

> [...]
> +SEC("tp_btf/task_newtask")
> +__failure
> +__msg("variable trusted_ptr_ access var_off=(0x0; 0xffffffffffffffff) disallowed")
> +int BPF_PROG(trusted_type_match_mismatch_neg_var_off, struct task_struct *task,
> +            u64 clone_flags)
> +{
> +       s64 var_off = task->start_time;
> +       task = bpf_get_current_task_btf();
> +
> +       bpf_assert_range(var_off, -64, 64);
> +       /* Need one bpf_throw() reference, otherwise BTF gen fails. */
> +       if (!task)
> +               bpf_throw(1);

It seems s390x is failing because it doesn't have support for throw.
So I'd just drop this selftest, since on second thought I think it
doesn't provide any additional coverage than the pos var_off one.

> +
> +       task = (void *)task + var_off;
> +       /* Passing a trusted pointer with an incorrect variable offset, type
> +        * match will succeed due to reg->off == 0, but the later call to
> +        * __check_ptr_off_reg should fail as it's responsible for checking
> +        * reg->var_off.
> +        */
> +       task = bpf_task_acquire(task);
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.45.2.627.g7a2c4fd464-goog
>
> /M

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 6816ff064516..e315bd0a1502 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -87,6 +87,7 @@ 
 #include "verifier_xdp.skel.h"
 #include "verifier_xdp_direct_packet_access.skel.h"
 #include "verifier_bits_iter.skel.h"
+#include "verifier_arg_reg_off_reject.skel.h"
 
 #define MAX_ENTRIES 11
 
@@ -204,6 +205,7 @@  void test_verifier_xadd(void)                 { RUN(verifier_xadd); }
 void test_verifier_xdp(void)                  { RUN(verifier_xdp); }
 void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
 void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
+void test_verifier_arg_reg_off_reject(void) { RUN(verifier_arg_reg_off_reject); }
 
 static int init_test_val_map(struct bpf_object *obj, char *map_name)
 {
diff --git a/tools/testing/selftests/bpf/progs/verifier_arg_reg_off_reject.c b/tools/testing/selftests/bpf/progs/verifier_arg_reg_off_reject.c
new file mode 100644
index 000000000000..b46656f4cb62
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_arg_reg_off_reject.c
@@ -0,0 +1,154 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Google LLC. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/limits.h>
+
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym;
+void bpf_task_release(struct task_struct *p) __ksym;
+
+struct random_type {
+	u64 id;
+	u64 ref;
+};
+
+struct alloc_type {
+	u64 id;
+	struct nested_type {
+		u64 id;
+	} n;
+	struct random_type __kptr *r;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+	__uint(max_entries, 256 * 1024);
+} ringbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, struct alloc_type);
+	__uint(max_entries, 1);
+} array_map SEC(".maps");
+
+SEC("tc")
+__failure
+__msg("R1 must have a fixed offset of 0 when passed to a OBJ_RELEASE/KF_RELEASE flagged BPF helper/kfunc which takes a void *")
+int alloc_obj_release(void *ctx)
+{
+	struct alloc_type *a;
+
+	a = bpf_obj_new(typeof(*a));
+	if (!a) {
+		return 0;
+	}
+	/* bpf_obj_drop_impl() takes a void *, so when we attempt to pass in
+	 * something with a reg->off, it should be rejected as we expect to have
+	 * the original pointer passed to the respective BPF helper unmodified.
+	 */
+	bpf_obj_drop(&a->n);
+	return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure
+__msg("R1 must have a fixed offset of 0 when passed to a OBJ_RELEASE/KF_RELEASE flagged BPF helper/kfunc which takes a void *")
+int BPF_PROG(mem_obj_release, struct file *file)
+{
+	int ret;
+	char *buf;
+
+	buf = bpf_ringbuf_reserve(&ringbuf, PATH_MAX, 0);
+	if (!buf)
+		return 0;
+
+	ret = bpf_d_path(&file->f_path, buf, PATH_MAX);
+	if (ret <= 0) {
+		bpf_ringbuf_discard(buf += 8, 0);
+		return 0;
+	}
+
+	bpf_ringbuf_submit(buf += 8, 0);
+	return 0;
+}
+
+SEC("tp_btf/task_newtask")
+__failure
+__msg("dereference of modified ptr_ ptr R1 off=44 disallowed")
+__msg("R1 must have a fixed offset of 0 when passed to a OBJ_RELEASE/KF_RELEASE flagged BPF helper/kfunc which takes a void *")
+int BPF_PROG(type_match_mismatch, struct task_struct *task,
+	     u64 clone_flags)
+{
+	struct task_struct *acquired;
+
+	acquired = bpf_task_acquire(bpf_get_current_task_btf());
+	if (!acquired)
+		return 0;
+
+	bpf_task_release((struct task_struct *)&acquired->flags);
+	return 0;
+}
+
+SEC("tp_btf/task_newtask")
+__failure
+__msg("kernel function bpf_task_acquire args#0 expected pointer to STRUCT task_struct")
+int BPF_PROG(trusted_type_match_mismatch, struct task_struct *task,
+	     u64 clone_flags)
+{
+	/* Passing a trusted pointer with incorrect offset will result in a type
+	 * mismatch.
+	 */
+	bpf_task_acquire((struct task_struct *)&bpf_get_current_task_btf()->flags);
+	return 0;
+}
+
+SEC("tp_btf/task_newtask")
+__failure
+__msg("variable trusted_ptr_ access var_off=(0x0; 0xffffffff) disallowed")
+int BPF_PROG(trusted_type_match_mismatch_var_off, struct task_struct *task,
+	     u64 clone_flags)
+{
+	u32 var_off = bpf_get_prandom_u32();
+	task = bpf_get_current_task_btf();
+
+	task = (void *)task + var_off;
+	/* Passing a trusted pointer with an incorrect variable offset, type
+	 * match will succeed due to reg->off == 0, but the later call to
+	 * __check_ptr_off_reg should fail as it's responsible for checking
+	 * reg->var_off.
+	 */
+	bpf_task_acquire(task);
+	return 0;
+}
+
+SEC("tp_btf/task_newtask")
+__failure
+__msg("variable trusted_ptr_ access var_off=(0x0; 0xffffffffffffffff) disallowed")
+int BPF_PROG(trusted_type_match_mismatch_neg_var_off, struct task_struct *task,
+	     u64 clone_flags)
+{
+	s64 var_off = task->start_time;
+	task = bpf_get_current_task_btf();
+
+	bpf_assert_range(var_off, -64, 64);
+	/* Need one bpf_throw() reference, otherwise BTF gen fails. */
+	if (!task)
+		bpf_throw(1);
+
+	task = (void *)task + var_off;
+	/* Passing a trusted pointer with an incorrect variable offset, type
+	 * match will succeed due to reg->off == 0, but the later call to
+	 * __check_ptr_off_reg should fail as it's responsible for checking
+	 * reg->var_off.
+	 */
+	task = bpf_task_acquire(task);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";