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

[PATCH bpf-next v1 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: 34d4ef5775f7("bpf: Add dynptr data slices");
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 kernel/bpf/verifier.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c59c3df0fea6..00f9b5a77734 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7341,6 +7341,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			}
 		}
 		break;
+	case BPF_FUNC_dynptr_data:
+		/* Find the id of the dynptr we're tracking the reference of.
+		 * We must do this before we reset caller saved regs.
+		 *
+		 * Please note as well that meta.ref_obj_id after the check_func_arg() calls doesn't
+		 * already contain the dynptr ref obj id, since dynptrs are stored on the stack.
+		 */
+		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: multiple refcounted args in func\n");
+					return -EFAULT;
+				}
+				meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
+			}
+		}
 	}
 
 	if (err)
@@ -7470,20 +7486,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		/* For release_reference() */
 		regs[BPF_REG_0].ref_obj_id = id;
 	} else if (func_id == BPF_FUNC_dynptr_data) {
-		int dynptr_id = 0, i;
-
-		/* Find the id of the dynptr we're acquiring a reference to */
-		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;
+		regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
 	}
 
 	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
-- 
2.30.2

[PATCH bpf-next v1 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>
---
 .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +-
 .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++-
 2 files changed, 33 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 d811cff73597..d8c4ed3ee146 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/sys_nanosleep")
-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,36 @@ int data_slice_use_after_release(void *ctx)
 	return 0;
 }
 
+SEC("?raw_tp/sys_nanosleep")
+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/sys_nanosleep")
 int data_slice_missing_null_check1(void *ctx)
-- 
2.30.2

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

[Jiri Olsa]

On Wed, Jul 20, 2022 at 07:48:20PM -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: 34d4ef5775f7("bpf: Add dynptr data slices");
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

LGTM

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  kernel/bpf/verifier.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..00f9b5a77734 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7341,6 +7341,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  			}
>  		}
>  		break;
> +	case BPF_FUNC_dynptr_data:
> +		/* Find the id of the dynptr we're tracking the reference of.
> +		 * We must do this before we reset caller saved regs.
> +		 *
> +		 * Please note as well that meta.ref_obj_id after the check_func_arg() calls doesn't
> +		 * already contain the dynptr ref obj id, since dynptrs are stored on the stack.
> +		 */
> +		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: multiple refcounted args in func\n");
> +					return -EFAULT;
> +				}
> +				meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> +			}
> +		}
>  	}
>  
>  	if (err)
> @@ -7470,20 +7486,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		/* For release_reference() */
>  		regs[BPF_REG_0].ref_obj_id = id;
>  	} else if (func_id == BPF_FUNC_dynptr_data) {
> -		int dynptr_id = 0, i;
> -
> -		/* Find the id of the dynptr we're acquiring a reference to */
> -		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;
> +		regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
>  	}
>  
>  	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
> -- 
> 2.30.2
> 

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

[Martin KaFai Lau]

On Wed, Jul 20, 2022 at 07:48:20PM -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: 34d4ef5775f7("bpf: Add dynptr data slices");
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  kernel/bpf/verifier.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..00f9b5a77734 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7341,6 +7341,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  			}
>  		}
>  		break;
> +	case BPF_FUNC_dynptr_data:
> +		/* Find the id of the dynptr we're tracking the reference of.
> +		 * We must do this before we reset caller saved regs.
> +		 *
> +		 * Please note as well that meta.ref_obj_id after the check_func_arg() calls doesn't
> +		 * already contain the dynptr ref obj id, since dynptrs are stored on the stack.
> +		 */
> +		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: multiple refcounted args in func\n");
> +					return -EFAULT;
> +				}
> +				meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
check_func_arg() is setting meta->ref_obj_id for each arg.
Can this meta.ref_obj_id assignment be done in check_func_arg()
instead of looping all args again here.

> +			}
> +		}
>  	}
>  
>  	if (err)
> @@ -7470,20 +7486,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		/* For release_reference() */
>  		regs[BPF_REG_0].ref_obj_id = id;
>  	} else if (func_id == BPF_FUNC_dynptr_data) {
> -		int dynptr_id = 0, i;
> -
> -		/* Find the id of the dynptr we're acquiring a reference to */
> -		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;
> +		regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
nit. This will be the same as the earlier is_ptr_cast_function().
Merge the if statements ?

>  	}
>  
>  	do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
> -- 
> 2.30.2
> 

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

[Hao Luo]

Hi Joanne,

On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> 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>
> ---
>  .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +-
>  .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++-
>  2 files changed, 33 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 d811cff73597..d8c4ed3ee146 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/sys_nanosleep")
> -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,36 @@ int data_slice_use_after_release(void *ctx)
>         return 0;
>  }
>
> +SEC("?raw_tp/sys_nanosleep")
> +int data_slice_use_after_release2(void *ctx)

Could you put comments explaining the reason for failure, like other test cases?

> +{
> +       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;
> +}
> +

Joanne, I haven't been following the effort of dynptr, so I am still
learning dynptr. Is there any use of `ptr1` in this test case?

>  /* A data slice must be first checked for NULL */
>  SEC("?raw_tp/sys_nanosleep")
>  int data_slice_missing_null_check1(void *ctx)
> --
> 2.30.2
>

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

[Joanne Koong]

On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@google.com> wrote:
>
> Hi Joanne,
>
> On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > 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>
> > ---
> >  .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +-
> >  .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++-
> >  2 files changed, 33 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 d811cff73597..d8c4ed3ee146 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/sys_nanosleep")
> > -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,36 @@ int data_slice_use_after_release(void *ctx)
> >         return 0;
> >  }
> >
> > +SEC("?raw_tp/sys_nanosleep")
> > +int data_slice_use_after_release2(void *ctx)
>
> Could you put comments explaining the reason for failure, like other test cases?
>
Hi Hao. The explanation for the data_slice_use_after_release test
cases is above the "data_slice_use_after_release1" case, but I can
also copy/paste that comment to above "data_slice_use_after_release2"
as well to make it easier to spot. I'll do that for v2.
> > +{
> > +       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;
> > +}
> > +
>
> Joanne, I haven't been following the effort of dynptr, so I am still
> learning dynptr. Is there any use of `ptr1` in this test case?

The use of ptr1 is so that ptr2 will be at a non-zero offset from the
frame pointer. This bug previously was unspotted because we were only
testing invalidated data slices for ptrs that were at a zero offset.

I will include a comment about this in the test to make it more clear :)

>
> >  /* A data slice must be first checked for NULL */
> >  SEC("?raw_tp/sys_nanosleep")
> >  int data_slice_missing_null_check1(void *ctx)
> > --
> > 2.30.2
> >

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

[Joanne Koong]

On Thu, Jul 21, 2022 at 10:02 AM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Wed, Jul 20, 2022 at 07:48:20PM -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: 34d4ef5775f7("bpf: Add dynptr data slices");
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  kernel/bpf/verifier.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index c59c3df0fea6..00f9b5a77734 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -7341,6 +7341,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >                       }
> >               }
> >               break;
> > +     case BPF_FUNC_dynptr_data:
> > +             /* Find the id of the dynptr we're tracking the reference of.
> > +              * We must do this before we reset caller saved regs.
> > +              *
> > +              * Please note as well that meta.ref_obj_id after the check_func_arg() calls doesn't
> > +              * already contain the dynptr ref obj id, since dynptrs are stored on the stack.
> > +              */
> > +             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: multiple refcounted args in func\n");
> > +                                     return -EFAULT;
> > +                             }
> > +                             meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> check_func_arg() is setting meta->ref_obj_id for each arg.
> Can this meta.ref_obj_id assignment be done in check_func_arg()
> instead of looping all args again here.
>
I think so! Will update for v2.
> > +                     }
> > +             }
> >       }
> >
> >       if (err)
> > @@ -7470,20 +7486,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >               /* For release_reference() */
> >               regs[BPF_REG_0].ref_obj_id = id;
> >       } else if (func_id == BPF_FUNC_dynptr_data) {
> > -             int dynptr_id = 0, i;
> > -
> > -             /* Find the id of the dynptr we're acquiring a reference to */
> > -             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;
> > +             regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> nit. This will be the same as the earlier is_ptr_cast_function().
> Merge the if statements ?
Will do for v2
>
> >       }
> >
> >       do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
Thanks for taking a look at this patch, Jiri and Martin!
> > --
> > 2.30.2
> >

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

[Hao Luo]

On Fri, Jul 22, 2022 at 9:40 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@google.com> wrote:
> >
> > Hi Joanne,
> >
> > On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > 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>
> > > ---
> > >  .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +-
> > >  .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++-
> > >  2 files changed, 33 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 d811cff73597..d8c4ed3ee146 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/sys_nanosleep")
> > > -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,36 @@ int data_slice_use_after_release(void *ctx)
> > >         return 0;
> > >  }
> > >
> > > +SEC("?raw_tp/sys_nanosleep")
> > > +int data_slice_use_after_release2(void *ctx)
> >
> > Could you put comments explaining the reason for failure, like other test cases?
> >
> Hi Hao. The explanation for the data_slice_use_after_release test
> cases is above the "data_slice_use_after_release1" case, but I can
> also copy/paste that comment to above "data_slice_use_after_release2"
> as well to make it easier to spot. I'll do that for v2.

Thanks Joanne, appreciate the comment about the cause of failure! I
noticed that in other test cases like

ringbuf_missing_release1()
ringbuf_missing_release2()

There is a general description before the release1, and more details
inside each function. Maybe following that convention is better? Sorry
for having many requests from me. :)

> > > +{
> > > +       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;
> > > +}
> > > +
> >
> > Joanne, I haven't been following the effort of dynptr, so I am still
> > learning dynptr. Is there any use of `ptr1` in this test case?
>
> The use of ptr1 is so that ptr2 will be at a non-zero offset from the
> frame pointer. This bug previously was unspotted because we were only
> testing invalidated data slices for ptrs that were at a zero offset.
>
> I will include a comment about this in the test to make it more clear :)
>

Sounds good. It would be great if we can fix that bug and spare the
use of ptr1 here.

Thanks!

> >
> > >  /* A data slice must be first checked for NULL */
> > >  SEC("?raw_tp/sys_nanosleep")
> > >  int data_slice_missing_null_check1(void *ctx)
> > > --
> > > 2.30.2
> > >

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

[Hao Luo]

On Fri, Jul 22, 2022 at 12:25 PM Hao Luo <haoluo@google.com> wrote:
>
> On Fri, Jul 22, 2022 at 9:40 AM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@google.com> wrote:
> > >
> > > Hi Joanne,
> > >
> > > On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > >
> > > > 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>
> > > > ---
> > > >  .../testing/selftests/bpf/prog_tests/dynptr.c |  3 +-
> > > >  .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++-
> > > >  2 files changed, 33 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 d811cff73597..d8c4ed3ee146 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/sys_nanosleep")
> > > > -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,36 @@ int data_slice_use_after_release(void *ctx)
> > > >         return 0;
> > > >  }
> > > >
> > > > +SEC("?raw_tp/sys_nanosleep")
> > > > +int data_slice_use_after_release2(void *ctx)
> > >
> > > Could you put comments explaining the reason for failure, like other test cases?
> > >
> > Hi Hao. The explanation for the data_slice_use_after_release test
> > cases is above the "data_slice_use_after_release1" case, but I can
> > also copy/paste that comment to above "data_slice_use_after_release2"
> > as well to make it easier to spot. I'll do that for v2.
>
> Thanks Joanne, appreciate the comment about the cause of failure! I
> noticed that in other test cases like
>
> ringbuf_missing_release1()
> ringbuf_missing_release2()
>
> There is a general description before the release1, and more details
> inside each function. Maybe following that convention is better? Sorry
> for having many requests from me. :)
>
> > > > +{
> > > > +       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;
> > > > +}
> > > > +
> > >
> > > Joanne, I haven't been following the effort of dynptr, so I am still
> > > learning dynptr. Is there any use of `ptr1` in this test case?
> >
> > The use of ptr1 is so that ptr2 will be at a non-zero offset from the
> > frame pointer. This bug previously was unspotted because we were only
> > testing invalidated data slices for ptrs that were at a zero offset.
> >
> > I will include a comment about this in the test to make it more clear :)
> >
>
> Sounds good. It would be great if we can fix that bug and spare the
> use of ptr1 here.

Sorry, I don't mean we should do it in this patchset.

>
> Thanks!
>
> > >
> > > >  /* A data slice must be first checked for NULL */
> > > >  SEC("?raw_tp/sys_nanosleep")
> > > >  int data_slice_missing_null_check1(void *ctx)
> > > > --
> > > > 2.30.2
> > > >