* [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release
2022-07-22 17:58 [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Joanne Koong
@ 2022-07-22 17:58 ` Joanne Koong
2022-07-25 19:15 ` Martin KaFai Lau
2022-07-25 19:10 ` [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Martin KaFai Lau
2022-08-08 21:14 ` David Vernet
2 siblings, 1 reply; 7+ messages in thread
From: Joanne Koong @ 2022-07-22 17:58 UTC (permalink / raw)
To: bpf; +Cc: kafai, jolsa, haoluo, andrii, daniel, ast, 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 | 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 d811cff73597..a8ee58ba3a84 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,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/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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release
2022-07-22 17:58 ` [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release Joanne Koong
@ 2022-07-25 19:15 ` Martin KaFai Lau
0 siblings, 0 replies; 7+ messages in thread
From: Martin KaFai Lau @ 2022-07-25 19:15 UTC (permalink / raw)
To: Joanne Koong; +Cc: bpf, jolsa, haoluo, andrii, daniel, ast
On Fri, Jul 22, 2022 at 10:58:07AM -0700, Joanne Koong 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.
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier
2022-07-22 17:58 [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Joanne Koong
2022-07-22 17:58 ` [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release Joanne Koong
@ 2022-07-25 19:10 ` Martin KaFai Lau
2022-07-25 21:52 ` Joanne Koong
2022-08-08 21:14 ` David Vernet
2 siblings, 1 reply; 7+ messages in thread
From: Martin KaFai Lau @ 2022-07-25 19:10 UTC (permalink / raw)
To: Joanne Koong; +Cc: bpf, jolsa, haoluo, andrii, daniel, ast
On Fri, Jul 22, 2022 at 10:58:06AM -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>
> ---
> kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
> 1 file changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..29987b2ea26f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
>
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> - const struct bpf_func_proto *fn)
> + const struct bpf_func_proto *fn,
> + int func_id)
> {
> u32 regno = BPF_REG_1 + arg;
> struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> }
>
> meta->uninit_dynptr_regno = regno;
> - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> - const char *err_extra = "";
> + } else {
> + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> + const char *err_extra = "";
>
> - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> - case DYNPTR_TYPE_LOCAL:
> - err_extra = "local ";
> - break;
> - case DYNPTR_TYPE_RINGBUF:
> - err_extra = "ringbuf ";
> - break;
> - default:
> - break;
> - }
> + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> + case DYNPTR_TYPE_LOCAL:
> + err_extra = "local ";
> + break;
> + case DYNPTR_TYPE_RINGBUF:
> + err_extra = "ringbuf ";
> + break;
> + default:
> + break;
> + }
>
> - verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> - err_extra, arg + 1);
> - return -EINVAL;
> + verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> + err_extra, arg + 1);
> + return -EINVAL;
> + }
> + if (func_id == BPF_FUNC_dynptr_data) {
> + if (meta->ref_obj_id) {
> + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
If 'func_id == BPF_FUNC_dynptr_data' is not checked first,
this verbose (or the earlier one in the 'if (reg->ref_obj_id) {...}')
may be hit for the bpf_dynptr_write helper?
Overall lgtm.
Acked-by: Martin KaFai Lau <kafai@fb.com>
> + return -EFAULT;
> + }
> + /* Find the id of the dynptr we're tracking the reference of */
> + meta->ref_obj_id = stack_slot_get_id(env, reg);
> + }
> }
> break;
> case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> meta.func_id = func_id;
> /* check args */
> for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> - err = check_func_arg(env, i, &meta, fn);
> + err = check_func_arg(env, i, &meta, fn, func_id);
> if (err)
> return err;
> }
> @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> if (type_may_be_null(regs[BPF_REG_0].type))
> regs[BPF_REG_0].id = ++env->id_gen;
>
> - if (is_ptr_cast_function(func_id)) {
> + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
> /* For release_reference() */
> regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> } else if (is_acquire_function(func_id, meta.map_ptr)) {
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier
2022-07-25 19:10 ` [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Martin KaFai Lau
@ 2022-07-25 21:52 ` Joanne Koong
0 siblings, 0 replies; 7+ messages in thread
From: Joanne Koong @ 2022-07-25 21:52 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, jolsa, Hao Luo, Andrii Nakryiko, Daniel Borkmann,
Alexei Starovoitov
On Mon, Jul 25, 2022 at 12:10 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Fri, Jul 22, 2022 at 10:58:06AM -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>
> > ---
> > kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
> > 1 file changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index c59c3df0fea6..29987b2ea26f 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
> >
> > static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > struct bpf_call_arg_meta *meta,
> > - const struct bpf_func_proto *fn)
> > + const struct bpf_func_proto *fn,
> > + int func_id)
> > {
> > u32 regno = BPF_REG_1 + arg;
> > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> > @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > }
> >
> > meta->uninit_dynptr_regno = regno;
> > - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> > - const char *err_extra = "";
> > + } else {
> > + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> > + const char *err_extra = "";
> >
> > - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > - case DYNPTR_TYPE_LOCAL:
> > - err_extra = "local ";
> > - break;
> > - case DYNPTR_TYPE_RINGBUF:
> > - err_extra = "ringbuf ";
> > - break;
> > - default:
> > - break;
> > - }
> > + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > + case DYNPTR_TYPE_LOCAL:
> > + err_extra = "local ";
> > + break;
> > + case DYNPTR_TYPE_RINGBUF:
> > + err_extra = "ringbuf ";
> > + break;
> > + default:
> > + break;
> > + }
> >
> > - verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> > - err_extra, arg + 1);
> > - return -EINVAL;
> > + verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> > + err_extra, arg + 1);
> > + return -EINVAL;
> > + }
> > + if (func_id == BPF_FUNC_dynptr_data) {
> > + if (meta->ref_obj_id) {
> > + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
> If 'func_id == BPF_FUNC_dynptr_data' is not checked first,
> this verbose (or the earlier one in the 'if (reg->ref_obj_id) {...}')
> may be hit for the bpf_dynptr_write helper?
If the 'func_id == BPF_FUNC_dynptr_data' is not checked first, the
bpf_dynptr_write helper may hit the verbose if the source it's writing
from is ref-counted (for example if the source is a ringbuf record).
bpf_dynptr_write doesn't trigger the earlier "if (reg->ref_obj_id)"
case when the source is ref-counted because the dynptr isn't stored in
a reg; the dynptr's refcount is stored on the stack since the dynptr
is stored on the stack, so in that case there is only 1
reg->ref_obj_id (belonging to the src) found for bpf_dynptr_write.
>
> Overall lgtm.
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
>
> > + return -EFAULT;
> > + }
> > + /* Find the id of the dynptr we're tracking the reference of */
> > + meta->ref_obj_id = stack_slot_get_id(env, reg);
> > + }
> > }
> > break;
> > case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> > @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > meta.func_id = func_id;
> > /* check args */
> > for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> > - err = check_func_arg(env, i, &meta, fn);
> > + err = check_func_arg(env, i, &meta, fn, func_id);
> > if (err)
> > return err;
> > }
> > @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > if (type_may_be_null(regs[BPF_REG_0].type))
> > regs[BPF_REG_0].id = ++env->id_gen;
> >
> > - if (is_ptr_cast_function(func_id)) {
> > + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
> > /* For release_reference() */
> > regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> > } else if (is_acquire_function(func_id, meta.map_ptr)) {
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier
2022-07-22 17:58 [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Joanne Koong
2022-07-22 17:58 ` [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release Joanne Koong
2022-07-25 19:10 ` [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Martin KaFai Lau
@ 2022-08-08 21:14 ` David Vernet
2022-08-08 23:11 ` Joanne Koong
2 siblings, 1 reply; 7+ messages in thread
From: David Vernet @ 2022-08-08 21:14 UTC (permalink / raw)
To: Joanne Koong; +Cc: bpf, kafai, jolsa, haoluo, andrii, daniel, ast
On Fri, Jul 22, 2022 at 10:58:06AM -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>
> ---
Hi Joanne,
Overall this looks great, thanks. Just a couple small comments / questions.
> kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
> 1 file changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..29987b2ea26f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
>
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> - const struct bpf_func_proto *fn)
> + const struct bpf_func_proto *fn,
> + int func_id)
Can we get the func_id from meta instead of adding another argument? It
looks like the func_id is stored there before we call check_func_arg.
> {
> u32 regno = BPF_REG_1 + arg;
> struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> }
>
> meta->uninit_dynptr_regno = regno;
> - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> - const char *err_extra = "";
> + } else {
> + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> + const char *err_extra = "";
>
> - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> - case DYNPTR_TYPE_LOCAL:
> - err_extra = "local ";
> - break;
> - case DYNPTR_TYPE_RINGBUF:
> - err_extra = "ringbuf ";
> - break;
> - default:
> - break;
> - }
> + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> + case DYNPTR_TYPE_LOCAL:
> + err_extra = "local ";
> + break;
> + case DYNPTR_TYPE_RINGBUF:
> + err_extra = "ringbuf ";
> + break;
> + default:
> + break;
> + }
>
> - verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> - err_extra, arg + 1);
> - return -EINVAL;
> + verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> + err_extra, arg + 1);
> + return -EINVAL;
> + }
> + if (func_id == BPF_FUNC_dynptr_data) {
> + if (meta->ref_obj_id) {
> + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
> + return -EFAULT;
> + }
> + /* Find the id of the dynptr we're tracking the reference of */
> + meta->ref_obj_id = stack_slot_get_id(env, reg);
> + }
> }
> break;
> case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> meta.func_id = func_id;
> /* check args */
> for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> - err = check_func_arg(env, i, &meta, fn);
> + err = check_func_arg(env, i, &meta, fn, func_id);
> if (err)
> return err;
> }
> @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> if (type_may_be_null(regs[BPF_REG_0].type))
> regs[BPF_REG_0].id = ++env->id_gen;
>
> - if (is_ptr_cast_function(func_id)) {
> + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
Just a nit and my two cents, but IMO, is_ptr_cast_function() feels like a
bit of an unclear function name. It's only used for this specific if
statement, so maybe we should change that function name to something like
is_meta_stored_ref() and just add BPF_FUNC_dynptr_data to that list?
> /* 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 +7480,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 (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, ®s[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
>
Looks good otherwise, as mentioned above.
Thanks,
David
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier
2022-08-08 21:14 ` David Vernet
@ 2022-08-08 23:11 ` Joanne Koong
0 siblings, 0 replies; 7+ messages in thread
From: Joanne Koong @ 2022-08-08 23:11 UTC (permalink / raw)
To: David Vernet; +Cc: bpf, kafai, jolsa, haoluo, andrii, daniel, ast
On Mon, Aug 8, 2022 at 2:14 PM David Vernet <void@manifault.com> wrote:
>
> On Fri, Jul 22, 2022 at 10:58:06AM -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>
> > ---
>
> Hi Joanne,
>
> Overall this looks great, thanks. Just a couple small comments / questions.
>
> > kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
> > 1 file changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index c59c3df0fea6..29987b2ea26f 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
> >
> > static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > struct bpf_call_arg_meta *meta,
> > - const struct bpf_func_proto *fn)
> > + const struct bpf_func_proto *fn,
> > + int func_id)
>
> Can we get the func_id from meta instead of adding another argument? It
> looks like the func_id is stored there before we call check_func_arg.
Great idea! I didn't realize the func id is already stored in meta :)
Btw, for v3, I'm planning to move this logic out of check_func_arg,
and instead to the end of the "switch (func_id)" statement in
check_helper_call(). I think keeping check_func_arg() free of checking
func ids ends up being logically cleaner. Will send v3 out shortly
>
> > {
> > u32 regno = BPF_REG_1 + arg;
> > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> > @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > }
> >
> > meta->uninit_dynptr_regno = regno;
> > - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> > - const char *err_extra = "";
> > + } else {
> > + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> > + const char *err_extra = "";
> >
> > - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > - case DYNPTR_TYPE_LOCAL:
> > - err_extra = "local ";
> > - break;
> > - case DYNPTR_TYPE_RINGBUF:
> > - err_extra = "ringbuf ";
> > - break;
> > - default:
> > - break;
> > - }
> > + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > + case DYNPTR_TYPE_LOCAL:
> > + err_extra = "local ";
> > + break;
> > + case DYNPTR_TYPE_RINGBUF:
> > + err_extra = "ringbuf ";
> > + break;
> > + default:
> > + break;
> > + }
> >
> > - verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> > - err_extra, arg + 1);
> > - return -EINVAL;
> > + verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> > + err_extra, arg + 1);
> > + return -EINVAL;
> > + }
> > + if (func_id == BPF_FUNC_dynptr_data) {
> > + if (meta->ref_obj_id) {
> > + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
> > + return -EFAULT;
> > + }
> > + /* Find the id of the dynptr we're tracking the reference of */
> > + meta->ref_obj_id = stack_slot_get_id(env, reg);
> > + }
> > }
> > break;
> > case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> > @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > meta.func_id = func_id;
> > /* check args */
> > for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> > - err = check_func_arg(env, i, &meta, fn);
> > + err = check_func_arg(env, i, &meta, fn, func_id);
> > if (err)
> > return err;
> > }
> > @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > if (type_may_be_null(regs[BPF_REG_0].type))
> > regs[BPF_REG_0].id = ++env->id_gen;
> >
> > - if (is_ptr_cast_function(func_id)) {
> > + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
>
> Just a nit and my two cents, but IMO, is_ptr_cast_function() feels like a
> bit of an unclear function name. It's only used for this specific if
> statement, so maybe we should change that function name to something like
> is_meta_stored_ref() and just add BPF_FUNC_dynptr_data to that list?
I think is_ptr_cast_function() is named that because it refers to the
class of functions whose only purpose is to cast the ptr and return it
back. is_ptr_cast_function() and bpf_dynptr_data() are similar in that
they need to make sure the ref obj id from the reference arg is copied
to the return reg's ref obj id - so maybe renaming it to something
like "copies_ref_obj_id" ends up being clearer?
>
> > /* 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 +7480,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 (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, ®s[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
> >
>
> Looks good otherwise, as mentioned above.
>
> Thanks,
> David
^ permalink raw reply [flat|nested] 7+ messages in thread