| Message ID | 20220721024821.251231-1-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v1,1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
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, ®s[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, ®s[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 >
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, ®s[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, ®s[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 >
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, ®s[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, ®s[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 > >
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, ®s[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, ®s[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);
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(-)