[PATCH bpf-next v4 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[PATCH bpf-next v4 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[Joanne Koong]

When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
the ref obj id of the dynptr must be found and then associated with the data
slice.

The ref obj id of the dynptr must be found *before* the caller saved regs are
reset. Without this fix, the ref obj id tracking is not correct for
dynptrs that are at an offset from the frame pointer.

Please also note that the data slice's ref obj id must be assigned after the
ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
zero-marked.

Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices");
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 kernel/bpf/verifier.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 01e7f48b4d8c..28b02dc67a2a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -504,7 +504,7 @@ static bool is_ptr_cast_function(enum bpf_func_id func_id)
 		func_id == BPF_FUNC_skc_to_tcp_request_sock;
 }
 
-static bool is_dynptr_acquire_function(enum bpf_func_id func_id)
+static bool is_dynptr_ref_function(enum bpf_func_id func_id)
 {
 	return func_id == BPF_FUNC_dynptr_data;
 }
@@ -518,7 +518,7 @@ static bool helper_multiple_ref_obj_use(enum bpf_func_id func_id,
 		ref_obj_uses++;
 	if (is_acquire_function(func_id, map))
 		ref_obj_uses++;
-	if (is_dynptr_acquire_function(func_id))
+	if (is_dynptr_ref_function(func_id))
 		ref_obj_uses++;
 
 	return ref_obj_uses > 1;
@@ -7320,6 +7320,23 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			}
 		}
 		break;
+	case BPF_FUNC_dynptr_data:
+		for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
+			if (arg_type_is_dynptr(fn->arg_type[i])) {
+				if (meta.ref_obj_id) {
+					verbose(env, "verifier internal error: meta.ref_obj_id already set\n");
+					return -EFAULT;
+				}
+				/* Find the id of the dynptr we're tracking the reference of */
+				meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
+				break;
+			}
+		}
+		if (i == MAX_BPF_FUNC_REG_ARGS) {
+			verbose(env, "verifier internal error: no dynptr in bpf_dynptr_data()\n");
+			return -EFAULT;
+		}
+		break;
 	}
 
 	if (err)
@@ -7457,7 +7474,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		return -EFAULT;
 	}
 
-	if (is_ptr_cast_function(func_id)) {
+	if (is_ptr_cast_function(func_id) || is_dynptr_ref_function(func_id)) {
 		/* For release_reference() */
 		regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
 	} else if (is_acquire_function(func_id, meta.map_ptr)) {
@@ -7469,21 +7486,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		regs[BPF_REG_0].id = id;
 		/* For release_reference() */
 		regs[BPF_REG_0].ref_obj_id = id;
-	} else if (is_dynptr_acquire_function(func_id)) {
-		int dynptr_id = 0, i;
-
-		/* Find the id of the dynptr we're tracking the reference of */
-		for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
-			if (arg_type_is_dynptr(fn->arg_type[i])) {
-				if (dynptr_id) {
-					verbose(env, "verifier internal error: multiple dynptr args in func\n");
-					return -EFAULT;
-				}
-				dynptr_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
-			}
-		}
-		/* For release_reference() */
-		regs[BPF_REG_0].ref_obj_id = dynptr_id;
 	}
 
 	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
-- 
2.30.2

[PATCH bpf-next v4 2/2] selftests/bpf: add extra test for using dynptr data slice after release

[Joanne Koong]

Add an additional test, "data_slice_use_after_release2", for ensuring
that data slices are correctly invalidated by the verifier after the
dynptr whose ref obj id they track is released. In particular, this
tests data slice invalidation for dynptrs located at a non-zero offset
from the frame pointer.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>
---
 .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +-
 .../testing/selftests/bpf/progs/dynptr_fail.c | 38 ++++++++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index 3c7aa82b98e2..bcf80b9f7c27 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -22,7 +22,8 @@ static struct {
 	{"add_dynptr_to_map2", "invalid indirect read from stack"},
 	{"data_slice_out_of_bounds_ringbuf", "value is outside of the allowed memory range"},
 	{"data_slice_out_of_bounds_map_value", "value is outside of the allowed memory range"},
-	{"data_slice_use_after_release", "invalid mem access 'scalar'"},
+	{"data_slice_use_after_release1", "invalid mem access 'scalar'"},
+	{"data_slice_use_after_release2", "invalid mem access 'scalar'"},
 	{"data_slice_missing_null_check1", "invalid mem access 'mem_or_null'"},
 	{"data_slice_missing_null_check2", "invalid mem access 'mem_or_null'"},
 	{"invalid_helper1", "invalid indirect read from stack"},
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index b5e0a87f0a36..b0f08ff024fb 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -248,7 +248,7 @@ int data_slice_out_of_bounds_map_value(void *ctx)
 
 /* A data slice can't be used after it has been released */
 SEC("?raw_tp")
-int data_slice_use_after_release(void *ctx)
+int data_slice_use_after_release1(void *ctx)
 {
 	struct bpf_dynptr ptr;
 	struct sample *sample;
@@ -272,6 +272,42 @@ int data_slice_use_after_release(void *ctx)
 	return 0;
 }
 
+/* A data slice can't be used after it has been released.
+ *
+ * This tests the case where the data slice tracks a dynptr (ptr2)
+ * that is at a non-zero offset from the frame pointer (ptr1 is at fp,
+ * ptr2 is at fp - 16).
+ */
+SEC("?raw_tp")
+int data_slice_use_after_release2(void *ctx)
+{
+	struct bpf_dynptr ptr1, ptr2;
+	struct sample *sample;
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr1);
+	bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(*sample), 0, &ptr2);
+
+	sample = bpf_dynptr_data(&ptr2, 0, sizeof(*sample));
+	if (!sample)
+		goto done;
+
+	sample->pid = 23;
+
+	bpf_ringbuf_submit_dynptr(&ptr2, 0);
+
+	/* this should fail */
+	sample->pid = 23;
+
+	bpf_ringbuf_submit_dynptr(&ptr1, 0);
+
+	return 0;
+
+done:
+	bpf_ringbuf_discard_dynptr(&ptr2, 0);
+	bpf_ringbuf_discard_dynptr(&ptr1, 0);
+	return 0;
+}
+
 /* A data slice must be first checked for NULL */
 SEC("?raw_tp")
 int data_slice_missing_null_check1(void *ctx)
-- 
2.30.2

Re: [PATCH bpf-next v4 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[David Vernet]

On Tue, Aug 09, 2022 at 02:40:54PM -0700, Joanne Koong wrote:
> When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> the ref obj id of the dynptr must be found and then associated with the data
> slice.
> 
> The ref obj id of the dynptr must be found *before* the caller saved regs are
> reset. Without this fix, the ref obj id tracking is not correct for
> dynptrs that are at an offset from the frame pointer.
> 
> Please also note that the data slice's ref obj id must be assigned after the
> ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
> zero-marked.
> 
> Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices");
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---

[...]

Looks good, thanks Joanne!

Acked-by: David Vernet <void@manifault.com>

Re: [PATCH bpf-next v4 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[Alexei Starovoitov]

On Tue, Aug 9, 2022 at 2:41 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> the ref obj id of the dynptr must be found and then associated with the data
> slice.
>
> The ref obj id of the dynptr must be found *before* the caller saved regs are
> reset. Without this fix, the ref obj id tracking is not correct for
> dynptrs that are at an offset from the frame pointer.
>
> Please also note that the data slice's ref obj id must be assigned after the
> ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
> zero-marked.
>
> Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices");

The proper format is:
Fixes: 34d4ef5775f7 ("bpf: Add dynptr data slices")
make sure you have abbrev = 12 in your .gitconfig
No need for ; at the end.

> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  kernel/bpf/verifier.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 01e7f48b4d8c..28b02dc67a2a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -504,7 +504,7 @@ static bool is_ptr_cast_function(enum bpf_func_id func_id)
>                 func_id == BPF_FUNC_skc_to_tcp_request_sock;
>  }
>
> -static bool is_dynptr_acquire_function(enum bpf_func_id func_id)
> +static bool is_dynptr_ref_function(enum bpf_func_id func_id)
>  {
>         return func_id == BPF_FUNC_dynptr_data;
>  }
> @@ -518,7 +518,7 @@ static bool helper_multiple_ref_obj_use(enum bpf_func_id func_id,
>                 ref_obj_uses++;
>         if (is_acquire_function(func_id, map))
>                 ref_obj_uses++;
> -       if (is_dynptr_acquire_function(func_id))
> +       if (is_dynptr_ref_function(func_id))
>                 ref_obj_uses++;
>
>         return ref_obj_uses > 1;
> @@ -7320,6 +7320,23 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                         }
>                 }
>                 break;
> +       case BPF_FUNC_dynptr_data:
> +               for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> +                       if (arg_type_is_dynptr(fn->arg_type[i])) {
> +                               if (meta.ref_obj_id) {
> +                                       verbose(env, "verifier internal error: meta.ref_obj_id already set\n");
> +                                       return -EFAULT;
> +                               }
> +                               /* Find the id of the dynptr we're tracking the reference of */
> +                               meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> +                               break;
> +                       }
> +               }
> +               if (i == MAX_BPF_FUNC_REG_ARGS) {
> +                       verbose(env, "verifier internal error: no dynptr in bpf_dynptr_data()\n");
> +                       return -EFAULT;
> +               }
> +               break;
>         }
>
>         if (err)
> @@ -7457,7 +7474,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                 return -EFAULT;
>         }
>
> -       if (is_ptr_cast_function(func_id)) {
> +       if (is_ptr_cast_function(func_id) || is_dynptr_ref_function(func_id)) {
>                 /* For release_reference() */
>                 regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
>         } else if (is_acquire_function(func_id, meta.map_ptr)) {
> @@ -7469,21 +7486,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                 regs[BPF_REG_0].id = id;
>                 /* For release_reference() */
>                 regs[BPF_REG_0].ref_obj_id = id;
> -       } else if (is_dynptr_acquire_function(func_id)) {
> -               int dynptr_id = 0, i;
> -
> -               /* Find the id of the dynptr we're tracking the reference of */
> -               for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> -                       if (arg_type_is_dynptr(fn->arg_type[i])) {
> -                               if (dynptr_id) {
> -                                       verbose(env, "verifier internal error: multiple dynptr args in func\n");
> -                                       return -EFAULT;
> -                               }
> -                               dynptr_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);

So this bit of code was just grabbing REG_[1-5] with reg->off == 0
and random spilled_ptr.id ?
It never worked correctly, right?

Technically bpf material, but applied to bpf-next due to conflicts.

Re: [PATCH bpf-next v4 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue,  9 Aug 2022 14:40:54 -0700 you wrote:
> When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> the ref obj id of the dynptr must be found and then associated with the data
> slice.
> 
> The ref obj id of the dynptr must be found *before* the caller saved regs are
> reset. Without this fix, the ref obj id tracking is not correct for
> dynptrs that are at an offset from the frame pointer.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v4,1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier
    https://git.kernel.org/bpf/bpf-next/c/883743422ced
  - [bpf-next,v4,2/2] selftests/bpf: add extra test for using dynptr data slice after release
    https://git.kernel.org/bpf/bpf-next/c/dc444be8bae4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html