Message ID | 20230301154953.641654-4-joannelkoong@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 1d18feb2c915c5ad0a9a61d04b8560e8efb78ce8 |
Delegated to: | BPF |
Headers | show |
Series | Add skb + xdp dynptrs | expand |
On Wed, Mar 01, 2023 at 04:49:46PM CET, Joanne Koong wrote: > This change allows kfuncs to take in an uninitialized dynptr as a > parameter. Before this change, only helper functions could successfully > use uninitialized dynptrs. This change moves the memory access check > (including stack state growing and slot marking) into > process_dynptr_func(), which both helpers and kfuncs call into. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > kernel/bpf/verifier.c | 67 ++++++++++++++----------------------------- > 1 file changed, 22 insertions(+), 45 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index e0e00509846b..82e39fc5ed05 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -268,7 +268,6 @@ struct bpf_call_arg_meta { > u32 ret_btf_id; > u32 subprogno; > struct btf_field *kptr_field; > - u8 uninit_dynptr_regno; > }; > > struct btf *btf_vmlinux; > @@ -6225,10 +6224,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument > * type, and declare it as 'const struct bpf_dynptr *' in their prototype. > */ > -static int process_dynptr_func(struct bpf_verifier_env *env, int regno, > - enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta) > +static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx, > + enum bpf_arg_type arg_type) > { > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > + int err; > > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > @@ -6254,23 +6254,23 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, > * to. > */ > if (arg_type & MEM_UNINIT) { > + int i; > + > if (!is_dynptr_reg_valid_uninit(env, reg)) { > verbose(env, "Dynptr has to be an uninitialized dynptr\n"); > return -EINVAL; > } > > - /* We only support one dynptr being uninitialized at the moment, > - * which is sufficient for the helper functions we have right now. > - */ > - if (meta->uninit_dynptr_regno) { > - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); > - return -EFAULT; > + /* we write BPF_DW bits (8 bytes) at a time */ > + for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { > + err = check_mem_access(env, insn_idx, regno, > + i, BPF_DW, BPF_WRITE, -1, false); > + if (err) > + return err; > } I am not sure moving check_mem_access into process_dynptr_func is the right thing to do. Not sure if a problem already, but sooner or later it might be. The side effects of the call should take effect on the current state only after we have gone through all arguments for the helper/kfunc call. In this case we will now do stack access while processing the dynptr arg, which may affect the state of stack we see through other memory arguments coming later. I think it is better to do it after argument processing is done, similar to existing meta.access_size handling which is done after check_func_arg loop (for the same reasons). > [...]
On Sun, Mar 5, 2023 at 11:36 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, Mar 01, 2023 at 04:49:46PM CET, Joanne Koong wrote: > > This change allows kfuncs to take in an uninitialized dynptr as a > > parameter. Before this change, only helper functions could successfully > > use uninitialized dynptrs. This change moves the memory access check > > (including stack state growing and slot marking) into > > process_dynptr_func(), which both helpers and kfuncs call into. > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > --- > > kernel/bpf/verifier.c | 67 ++++++++++++++----------------------------- > > 1 file changed, 22 insertions(+), 45 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index e0e00509846b..82e39fc5ed05 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -268,7 +268,6 @@ struct bpf_call_arg_meta { > > u32 ret_btf_id; > > u32 subprogno; > > struct btf_field *kptr_field; > > - u8 uninit_dynptr_regno; > > }; > > > > struct btf *btf_vmlinux; > > @@ -6225,10 +6224,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > > * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument > > * type, and declare it as 'const struct bpf_dynptr *' in their prototype. > > */ > > -static int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > - enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta) > > +static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx, > > + enum bpf_arg_type arg_type) > > { > > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > > + int err; > > > > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an > > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > > @@ -6254,23 +6254,23 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > * to. > > */ > > if (arg_type & MEM_UNINIT) { > > + int i; > > + > > if (!is_dynptr_reg_valid_uninit(env, reg)) { > > verbose(env, "Dynptr has to be an uninitialized dynptr\n"); > > return -EINVAL; > > } > > > > - /* We only support one dynptr being uninitialized at the moment, > > - * which is sufficient for the helper functions we have right now. > > - */ > > - if (meta->uninit_dynptr_regno) { > > - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); > > - return -EFAULT; > > + /* we write BPF_DW bits (8 bytes) at a time */ > > + for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { > > + err = check_mem_access(env, insn_idx, regno, > > + i, BPF_DW, BPF_WRITE, -1, false); > > + if (err) > > + return err; > > } > > I am not sure moving check_mem_access into process_dynptr_func is the right > thing to do. Not sure if a problem already, but sooner or later it might be. > > The side effects of the call should take effect on the current state only after > we have gone through all arguments for the helper/kfunc call. In this case we > will now do stack access while processing the dynptr arg, which may affect the > state of stack we see through other memory arguments coming later. > > I think it is better to do it after argument processing is done, similar to > existing meta.access_size handling which is done after check_func_arg loop (for > the same reasons). > Thanks for taking a look. I don't have a strong preference for either so if you do feel strongly about doing the check_mem_access() only after argument processing, I'm happy to change it. The check_mem_access() call on the dyntpr will mark only the dynptr stack slots, so I don't fully see how it may affect the state of stack through other memory arguments coming later, but I do see your point about keeping the logic more separated out. > > [...]
On Mon, Mar 6, 2023 at 10:54 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > On Sun, Mar 5, 2023 at 11:36 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Wed, Mar 01, 2023 at 04:49:46PM CET, Joanne Koong wrote: > > > This change allows kfuncs to take in an uninitialized dynptr as a > > > parameter. Before this change, only helper functions could successfully > > > use uninitialized dynptrs. This change moves the memory access check > > > (including stack state growing and slot marking) into > > > process_dynptr_func(), which both helpers and kfuncs call into. > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > > --- > > > kernel/bpf/verifier.c | 67 ++++++++++++++----------------------------- > > > 1 file changed, 22 insertions(+), 45 deletions(-) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index e0e00509846b..82e39fc5ed05 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -268,7 +268,6 @@ struct bpf_call_arg_meta { > > > u32 ret_btf_id; > > > u32 subprogno; > > > struct btf_field *kptr_field; > > > - u8 uninit_dynptr_regno; > > > }; > > > > > > struct btf *btf_vmlinux; > > > @@ -6225,10 +6224,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > > > * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument > > > * type, and declare it as 'const struct bpf_dynptr *' in their prototype. > > > */ > > > -static int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > > - enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta) > > > +static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx, > > > + enum bpf_arg_type arg_type) > > > { > > > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > > > + int err; > > > > > > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an > > > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > > > @@ -6254,23 +6254,23 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > > * to. > > > */ > > > if (arg_type & MEM_UNINIT) { > > > + int i; > > > + > > > if (!is_dynptr_reg_valid_uninit(env, reg)) { > > > verbose(env, "Dynptr has to be an uninitialized dynptr\n"); > > > return -EINVAL; > > > } > > > > > > - /* We only support one dynptr being uninitialized at the moment, > > > - * which is sufficient for the helper functions we have right now. > > > - */ > > > - if (meta->uninit_dynptr_regno) { > > > - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); > > > - return -EFAULT; > > > + /* we write BPF_DW bits (8 bytes) at a time */ > > > + for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { > > > + err = check_mem_access(env, insn_idx, regno, > > > + i, BPF_DW, BPF_WRITE, -1, false); > > > + if (err) > > > + return err; > > > } > > > > I am not sure moving check_mem_access into process_dynptr_func is the right > > thing to do. Not sure if a problem already, but sooner or later it might be. > > > > The side effects of the call should take effect on the current state only after > > we have gone through all arguments for the helper/kfunc call. In this case we > > will now do stack access while processing the dynptr arg, which may affect the > > state of stack we see through other memory arguments coming later. > > > > I think it is better to do it after argument processing is done, similar to > > existing meta.access_size handling which is done after check_func_arg loop (for > > the same reasons). > > > > Thanks for taking a look. I don't have a strong preference for either > so if you do feel strongly about doing the check_mem_access() only > after argument processing, I'm happy to change it. The > check_mem_access() call on the dyntpr will mark only the dynptr stack > slots, so I don't fully see how it may affect the state of stack > through other memory arguments coming later, but I do see your point > about keeping the logic more separated out. FWIW, I did a similar approach for iters as well. And I suspect it's not the only place where we do similar things while processing helper arguments, etc. Let's keep this in mind, but I wouldn't necessarily go complicating code right now with more of "let's record some info for later" and then "ok, we recorded something before, let's act on it". > > > > [...]
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e0e00509846b..82e39fc5ed05 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -268,7 +268,6 @@ struct bpf_call_arg_meta { u32 ret_btf_id; u32 subprogno; struct btf_field *kptr_field; - u8 uninit_dynptr_regno; }; struct btf *btf_vmlinux; @@ -6225,10 +6224,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument * type, and declare it as 'const struct bpf_dynptr *' in their prototype. */ -static int process_dynptr_func(struct bpf_verifier_env *env, int regno, - enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta) +static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx, + enum bpf_arg_type arg_type) { struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; + int err; /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): @@ -6254,23 +6254,23 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, * to. */ if (arg_type & MEM_UNINIT) { + int i; + if (!is_dynptr_reg_valid_uninit(env, reg)) { verbose(env, "Dynptr has to be an uninitialized dynptr\n"); return -EINVAL; } - /* We only support one dynptr being uninitialized at the moment, - * which is sufficient for the helper functions we have right now. - */ - if (meta->uninit_dynptr_regno) { - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); - return -EFAULT; + /* we write BPF_DW bits (8 bytes) at a time */ + for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { + err = check_mem_access(env, insn_idx, regno, + i, BPF_DW, BPF_WRITE, -1, false); + if (err) + return err; } - meta->uninit_dynptr_regno = regno; + err = mark_stack_slots_dynptr(env, reg, arg_type, insn_idx); } else /* MEM_RDONLY and None case from above */ { - int err; - /* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */ if (reg->type == CONST_PTR_TO_DYNPTR && !(arg_type & MEM_RDONLY)) { verbose(env, "cannot pass pointer to const bpf_dynptr, the helper mutates it\n"); @@ -6306,10 +6306,8 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, } err = mark_dynptr_read(env, reg); - if (err) - return err; } - return 0; + return err; } static bool arg_type_is_mem_size(enum bpf_arg_type type) @@ -6719,7 +6717,8 @@ static int dynptr_ref_obj_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 insn_idx) { u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; @@ -6932,7 +6931,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, err = check_mem_size_reg(env, reg, regno, true, meta); break; case ARG_PTR_TO_DYNPTR: - err = process_dynptr_func(env, regno, arg_type, meta); + err = process_dynptr_func(env, regno, insn_idx, arg_type); if (err) return err; break; @@ -8218,7 +8217,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, insn_idx); if (err) return err; } @@ -8243,30 +8242,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs = cur_regs(env); - /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot - * be reinitialized by any dynptr helper. Hence, mark_stack_slots_dynptr - * is safe to do directly. - */ - if (meta.uninit_dynptr_regno) { - if (regs[meta.uninit_dynptr_regno].type == CONST_PTR_TO_DYNPTR) { - verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be initialized\n"); - return -EFAULT; - } - /* we write BPF_DW bits (8 bytes) at a time */ - for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { - err = check_mem_access(env, insn_idx, meta.uninit_dynptr_regno, - i, BPF_DW, BPF_WRITE, -1, false); - if (err) - return err; - } - - err = mark_stack_slots_dynptr(env, ®s[meta.uninit_dynptr_regno], - fn->arg_type[meta.uninit_dynptr_regno - BPF_REG_1], - insn_idx); - if (err) - return err; - } - if (meta.release_regno) { err = -EINVAL; /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot @@ -9475,7 +9450,8 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, &meta->arg_rbtree_root.field); } -static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta) +static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta, + int insn_idx) { const char *func_name = meta->func_name, *ref_tname; const struct btf *btf = meta->btf; @@ -9672,7 +9648,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ return -EINVAL; } - ret = process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR | MEM_RDONLY, NULL); + ret = process_dynptr_func(env, regno, insn_idx, + ARG_PTR_TO_DYNPTR | MEM_RDONLY); if (ret < 0) return ret; break; @@ -9880,7 +9857,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, } /* Check the arguments */ - err = check_kfunc_args(env, &meta); + err = check_kfunc_args(env, &meta, insn_idx); if (err < 0) return err; /* In case of release function, we get register number of refcounted
This change allows kfuncs to take in an uninitialized dynptr as a parameter. Before this change, only helper functions could successfully use uninitialized dynptrs. This change moves the memory access check (including stack state growing and slot marking) into process_dynptr_func(), which both helpers and kfuncs call into. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- kernel/bpf/verifier.c | 67 ++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 45 deletions(-)