| Message ID | 20220809214055.4050604-1-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 883743422ced8c961ab05dc63ec81b75a4e56052 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v4,1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier | expand |
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>
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, ®s[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, ®s[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.
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!
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, ®s[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, ®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);
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(-)