* [PATCH bpf-next v1 0/2] Add __ref annotation for kfuncs @ 2022-07-27 8:15 Kumar Kartikeya Dwivedi 2022-07-27 8:15 ` [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args Kumar Kartikeya Dwivedi 2022-07-27 8:15 ` [PATCH bpf-next v1 2/2] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation Kumar Kartikeya Dwivedi 0 siblings, 2 replies; 13+ messages in thread From: Kumar Kartikeya Dwivedi @ 2022-07-27 8:15 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann We decided to delay adding this until a usecase came up, and since now there is one, instead of KF_TRUSTED_ARGS that applies to all arguments, add a __ref suffix that has the same effect but only for the argument it is applied to. Kumar Kartikeya Dwivedi (2): bpf: Add support for per-parameter trusted args selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation Documentation/bpf/kfuncs.rst | 18 +++++++++ kernel/bpf/btf.c | 39 +++++++++++++------- net/bpf/test_run.c | 9 ++++- tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++---- 4 files changed, 81 insertions(+), 23 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-27 8:15 [PATCH bpf-next v1 0/2] Add __ref annotation for kfuncs Kumar Kartikeya Dwivedi @ 2022-07-27 8:15 ` Kumar Kartikeya Dwivedi 2022-07-28 7:46 ` Roberto Sassu 2022-07-27 8:15 ` [PATCH bpf-next v1 2/2] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation Kumar Kartikeya Dwivedi 1 sibling, 1 reply; 13+ messages in thread From: Kumar Kartikeya Dwivedi @ 2022-07-27 8:15 UTC (permalink / raw) To: bpf; +Cc: Roberto Sassu, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Similar to how we detect mem, size pairs in kfunc, teach verifier to treat __ref suffix on argument name to imply that it must be a trusted arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag but limited to the specific parameter. This is required to ensure that kfunc that operate on some object only work on acquired pointers and not normal PTR_TO_BTF_ID with same type which can be obtained by pointer walking. Release functions need not specify such suffix on release arguments as they are already expected to receive one referenced argument. Cc: Roberto Sassu <roberto.sassu@huawei.com> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++ kernel/bpf/btf.c | 39 ++++++++++++++++++++++++------------ net/bpf/test_run.c | 9 +++++++-- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst index c0b7dae6dbf5..41dff6337446 100644 --- a/Documentation/bpf/kfuncs.rst +++ b/Documentation/bpf/kfuncs.rst @@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type of the pointer is used. Without __sz annotation, a kfunc cannot accept a void pointer. +2.2.2 __ref Annotation +---------------------- + +This annotation is used to indicate that the argument is trusted, i.e. it will +be a pointer from an acquire function (defined later), and its offset will be +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but +only on the parameter it is applied to. An example is shown below:: + + void bpf_task_send_signal(struct task_struct *task__ref, int signal) + { + ... + } + +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and +cannot be used on pointers obtained using pointer walking. This ensures that +caller always calls this kfunc on a task whose lifetime is guaranteed for the +duration of the call. + .. _BPF_kfunc_nodef: 2.3 Using an existing kernel function diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7ac971ea98d1..3ce9b2deef9c 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log, return true; } -static bool is_kfunc_arg_mem_size(const struct btf *btf, - const struct btf_param *arg, - const struct bpf_reg_state *reg) +static bool btf_param_match_suffix(const struct btf *btf, + const struct btf_param *arg, + const char *suffix) { - int len, sfx_len = sizeof("__sz") - 1; - const struct btf_type *t; + int len, sfx_len = strlen(suffix); const char *param_name; - t = btf_type_skip_modifiers(btf, arg->type, NULL); - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) - return false; - /* In the future, this can be ported to use BTF tagging */ param_name = btf_name_by_offset(btf, arg->name_off); if (str_is_empty(param_name)) @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, if (len < sfx_len) return false; param_name += len - sfx_len; - if (strncmp(param_name, "__sz", sfx_len)) + return !strncmp(param_name, suffix, sfx_len); +} + +static bool is_kfunc_arg_ref(const struct btf *btf, + const struct btf_param *arg) +{ + return btf_param_match_suffix(btf, arg, "__ref"); +} + +static bool is_kfunc_arg_mem_size(const struct btf *btf, + const struct btf_param *arg, + const struct bpf_reg_state *reg) +{ + const struct btf_type *t; + + t = btf_type_skip_modifiers(btf, arg->type, NULL); + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) return false; - return true; + return btf_param_match_suffix(btf, arg, "__sz"); } static int btf_check_func_arg_match(struct bpf_verifier_env *env, @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, u32 kfunc_flags) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); - bool rel = false, kptr_get = false, trusted_arg = false; + bool rel = false, kptr_get = false, kf_trusted_args = false; struct bpf_verifier_log *log = &env->log; u32 i, nargs, ref_id, ref_obj_id = 0; bool is_kfunc = btf_is_kernel(btf); @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, /* Only kfunc can be release func */ rel = kfunc_flags & KF_RELEASE; kptr_get = kfunc_flags & KF_KPTR_GET; - trusted_arg = kfunc_flags & KF_TRUSTED_ARGS; + kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS; } /* check that BTF function arguments match actual types that the @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, enum bpf_arg_type arg_type = ARG_DONTCARE; u32 regno = i + 1; struct bpf_reg_state *reg = ®s[regno]; + bool trusted_arg = false; t = btf_type_skip_modifiers(btf, args[i].type, NULL); if (btf_type_is_scalar(t)) { @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, /* Check if argument must be a referenced pointer, args + i has * been verified to be a pointer (after skipping modifiers). */ + trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i); if (is_kfunc && trusted_arg && !reg->ref_obj_id) { bpf_log(log, "R%d must be referenced\n", regno); return -EINVAL; diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index cbc9cd5058cb..247bfe52e585 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len) { } -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p) +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p) +{ +} + +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) { } @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3) BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1) BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1) BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2) -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref) BTF_SET8_END(test_sk_check_kfunc_ids) static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-27 8:15 ` [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args Kumar Kartikeya Dwivedi @ 2022-07-28 7:46 ` Roberto Sassu 2022-07-28 8:18 ` Roberto Sassu 0 siblings, 1 reply; 13+ messages in thread From: Roberto Sassu @ 2022-07-28 7:46 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > Sent: Wednesday, July 27, 2022 10:16 AM > Similar to how we detect mem, size pairs in kfunc, teach verifier to > treat __ref suffix on argument name to imply that it must be a trusted > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag > but limited to the specific parameter. This is required to ensure that > kfunc that operate on some object only work on acquired pointers and not > normal PTR_TO_BTF_ID with same type which can be obtained by pointer > walking. Release functions need not specify such suffix on release > arguments as they are already expected to receive one referenced > argument. Thanks, Kumar. I will try it. Roberto > Cc: Roberto Sassu <roberto.sassu@huawei.com> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++ > kernel/bpf/btf.c | 39 ++++++++++++++++++++++++------------ > net/bpf/test_run.c | 9 +++++++-- > 3 files changed, 51 insertions(+), 15 deletions(-) > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst > index c0b7dae6dbf5..41dff6337446 100644 > --- a/Documentation/bpf/kfuncs.rst > +++ b/Documentation/bpf/kfuncs.rst > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, > the size of the type > of the pointer is used. Without __sz annotation, a kfunc cannot accept a void > pointer. > > +2.2.2 __ref Annotation > +---------------------- > + > +This annotation is used to indicate that the argument is trusted, i.e. it will > +be a pointer from an acquire function (defined later), and its offset will be > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag > but > +only on the parameter it is applied to. An example is shown below:: > + > + void bpf_task_send_signal(struct task_struct *task__ref, int signal) > + { > + ... > + } > + > +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and > +cannot be used on pointers obtained using pointer walking. This ensures that > +caller always calls this kfunc on a task whose lifetime is guaranteed for the > +duration of the call. > + > .. _BPF_kfunc_nodef: > > 2.3 Using an existing kernel function > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 7ac971ea98d1..3ce9b2deef9c 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct > bpf_verifier_log *log, > return true; > } > > -static bool is_kfunc_arg_mem_size(const struct btf *btf, > - const struct btf_param *arg, > - const struct bpf_reg_state *reg) > +static bool btf_param_match_suffix(const struct btf *btf, > + const struct btf_param *arg, > + const char *suffix) > { > - int len, sfx_len = sizeof("__sz") - 1; > - const struct btf_type *t; > + int len, sfx_len = strlen(suffix); > const char *param_name; > > - t = btf_type_skip_modifiers(btf, arg->type, NULL); > - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > - return false; > - > /* In the future, this can be ported to use BTF tagging */ > param_name = btf_name_by_offset(btf, arg->name_off); > if (str_is_empty(param_name)) > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct btf > *btf, > if (len < sfx_len) > return false; > param_name += len - sfx_len; > - if (strncmp(param_name, "__sz", sfx_len)) > + return !strncmp(param_name, suffix, sfx_len); > +} > + > +static bool is_kfunc_arg_ref(const struct btf *btf, > + const struct btf_param *arg) > +{ > + return btf_param_match_suffix(btf, arg, "__ref"); > +} > + > +static bool is_kfunc_arg_mem_size(const struct btf *btf, > + const struct btf_param *arg, > + const struct bpf_reg_state *reg) > +{ > + const struct btf_type *t; > + > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > return false; > > - return true; > + return btf_param_match_suffix(btf, arg, "__sz"); > } > > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct > bpf_verifier_env *env, > u32 kfunc_flags) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > - bool rel = false, kptr_get = false, trusted_arg = false; > + bool rel = false, kptr_get = false, kf_trusted_args = false; > struct bpf_verifier_log *log = &env->log; > u32 i, nargs, ref_id, ref_obj_id = 0; > bool is_kfunc = btf_is_kernel(btf); > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct > bpf_verifier_env *env, > /* Only kfunc can be release func */ > rel = kfunc_flags & KF_RELEASE; > kptr_get = kfunc_flags & KF_KPTR_GET; > - trusted_arg = kfunc_flags & KF_TRUSTED_ARGS; > + kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS; > } > > /* check that BTF function arguments match actual types that the > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct > bpf_verifier_env *env, > enum bpf_arg_type arg_type = ARG_DONTCARE; > u32 regno = i + 1; > struct bpf_reg_state *reg = ®s[regno]; > + bool trusted_arg = false; > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > if (btf_type_is_scalar(t)) { > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct > bpf_verifier_env *env, > /* Check if argument must be a referenced pointer, args + i has > * been verified to be a pointer (after skipping modifiers). > */ > + trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i); > if (is_kfunc && trusted_arg && !reg->ref_obj_id) { > bpf_log(log, "R%d must be referenced\n", regno); > return -EINVAL; > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index cbc9cd5058cb..247bfe52e585 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 > *mem, int len) > { > } > > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p) > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p) > +{ > +} > + > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) > { > } > > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3) > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1) > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1) > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2) > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS) > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS) > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref) > BTF_SET8_END(test_sk_check_kfunc_ids) > > static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > -- > 2.34.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-28 7:46 ` Roberto Sassu @ 2022-07-28 8:18 ` Roberto Sassu 2022-07-28 8:45 ` Kumar Kartikeya Dwivedi 0 siblings, 1 reply; 13+ messages in thread From: Roberto Sassu @ 2022-07-28 8:18 UTC (permalink / raw) To: Roberto Sassu, Kumar Kartikeya Dwivedi, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > Sent: Thursday, July 28, 2022 9:46 AM > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > Sent: Wednesday, July 27, 2022 10:16 AM > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > treat __ref suffix on argument name to imply that it must be a trusted > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag > > but limited to the specific parameter. This is required to ensure that > > kfunc that operate on some object only work on acquired pointers and not > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer > > walking. Release functions need not specify such suffix on release > > arguments as they are already expected to receive one referenced > > argument. > > Thanks, Kumar. I will try it. Uhm. I realized that I was already using another suffix, __maybe_null, to indicate that a caller can pass NULL as argument. Wouldn't probably work well with two suffixes. Have you considered to extend BTF_ID_FLAGS to take five extra arguments, to set flags for each kfunc parameter? Thanks Roberto > Roberto > > > Cc: Roberto Sassu <roberto.sassu@huawei.com> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++ > > kernel/bpf/btf.c | 39 ++++++++++++++++++++++++------------ > > net/bpf/test_run.c | 9 +++++++-- > > 3 files changed, 51 insertions(+), 15 deletions(-) > > > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst > > index c0b7dae6dbf5..41dff6337446 100644 > > --- a/Documentation/bpf/kfuncs.rst > > +++ b/Documentation/bpf/kfuncs.rst > > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz > annotation, > > the size of the type > > of the pointer is used. Without __sz annotation, a kfunc cannot accept a void > > pointer. > > > > +2.2.2 __ref Annotation > > +---------------------- > > + > > +This annotation is used to indicate that the argument is trusted, i.e. it will > > +be a pointer from an acquire function (defined later), and its offset will be > > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc > flag > > but > > +only on the parameter it is applied to. An example is shown below:: > > + > > + void bpf_task_send_signal(struct task_struct *task__ref, int signal) > > + { > > + ... > > + } > > + > > +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and > > +cannot be used on pointers obtained using pointer walking. This ensures that > > +caller always calls this kfunc on a task whose lifetime is guaranteed for the > > +duration of the call. > > + > > .. _BPF_kfunc_nodef: > > > > 2.3 Using an existing kernel function > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 7ac971ea98d1..3ce9b2deef9c 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct > > bpf_verifier_log *log, > > return true; > > } > > > > -static bool is_kfunc_arg_mem_size(const struct btf *btf, > > - const struct btf_param *arg, > > - const struct bpf_reg_state *reg) > > +static bool btf_param_match_suffix(const struct btf *btf, > > + const struct btf_param *arg, > > + const char *suffix) > > { > > - int len, sfx_len = sizeof("__sz") - 1; > > - const struct btf_type *t; > > + int len, sfx_len = strlen(suffix); > > const char *param_name; > > > > - t = btf_type_skip_modifiers(btf, arg->type, NULL); > > - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > - return false; > > - > > /* In the future, this can be ported to use BTF tagging */ > > param_name = btf_name_by_offset(btf, arg->name_off); > > if (str_is_empty(param_name)) > > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct > btf > > *btf, > > if (len < sfx_len) > > return false; > > param_name += len - sfx_len; > > - if (strncmp(param_name, "__sz", sfx_len)) > > + return !strncmp(param_name, suffix, sfx_len); > > +} > > + > > +static bool is_kfunc_arg_ref(const struct btf *btf, > > + const struct btf_param *arg) > > +{ > > + return btf_param_match_suffix(btf, arg, "__ref"); > > +} > > + > > +static bool is_kfunc_arg_mem_size(const struct btf *btf, > > + const struct btf_param *arg, > > + const struct bpf_reg_state *reg) > > +{ > > + const struct btf_type *t; > > + > > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > return false; > > > > - return true; > > + return btf_param_match_suffix(btf, arg, "__sz"); > > } > > > > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct > > bpf_verifier_env *env, > > u32 kfunc_flags) > > { > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > - bool rel = false, kptr_get = false, trusted_arg = false; > > + bool rel = false, kptr_get = false, kf_trusted_args = false; > > struct bpf_verifier_log *log = &env->log; > > u32 i, nargs, ref_id, ref_obj_id = 0; > > bool is_kfunc = btf_is_kernel(btf); > > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct > > bpf_verifier_env *env, > > /* Only kfunc can be release func */ > > rel = kfunc_flags & KF_RELEASE; > > kptr_get = kfunc_flags & KF_KPTR_GET; > > - trusted_arg = kfunc_flags & KF_TRUSTED_ARGS; > > + kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS; > > } > > > > /* check that BTF function arguments match actual types that the > > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct > > bpf_verifier_env *env, > > enum bpf_arg_type arg_type = ARG_DONTCARE; > > u32 regno = i + 1; > > struct bpf_reg_state *reg = ®s[regno]; > > + bool trusted_arg = false; > > > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > > if (btf_type_is_scalar(t)) { > > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct > > bpf_verifier_env *env, > > /* Check if argument must be a referenced pointer, args + i has > > * been verified to be a pointer (after skipping modifiers). > > */ > > + trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i); > > if (is_kfunc && trusted_arg && !reg->ref_obj_id) { > > bpf_log(log, "R%d must be referenced\n", regno); > > return -EINVAL; > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > index cbc9cd5058cb..247bfe52e585 100644 > > --- a/net/bpf/test_run.c > > +++ b/net/bpf/test_run.c > > @@ -691,7 +691,11 @@ noinline void > bpf_kfunc_call_test_mem_len_fail2(u64 > > *mem, int len) > > { > > } > > > > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p) > > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p) > > +{ > > +} > > + > > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) > > { > > } > > > > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3) > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1) > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1) > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2) > > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS) > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS) > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref) > > BTF_SET8_END(test_sk_check_kfunc_ids) > > > > static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > > -- > > 2.34.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-28 8:18 ` Roberto Sassu @ 2022-07-28 8:45 ` Kumar Kartikeya Dwivedi 2022-07-28 9:02 ` Kumar Kartikeya Dwivedi 2022-07-28 9:02 ` Roberto Sassu 0 siblings, 2 replies; 13+ messages in thread From: Kumar Kartikeya Dwivedi @ 2022-07-28 8:45 UTC (permalink / raw) To: Roberto Sassu; +Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > Sent: Thursday, July 28, 2022 9:46 AM > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > treat __ref suffix on argument name to imply that it must be a trusted > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag > > > but limited to the specific parameter. This is required to ensure that > > > kfunc that operate on some object only work on acquired pointers and not > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer > > > walking. Release functions need not specify such suffix on release > > > arguments as they are already expected to receive one referenced > > > argument. > > > > Thanks, Kumar. I will try it. > > Uhm. I realized that I was already using another suffix, > __maybe_null, to indicate that a caller can pass NULL as > argument. > > Wouldn't probably work well with two suffixes. > Then you can maybe extend it to parse two suffixes at most (for now atleast)? > Have you considered to extend BTF_ID_FLAGS to take five > extra arguments, to set flags for each kfunc parameter? > I didn't understand this. Flags parameter is an OR of the flags you set, why would we want to extend it to take 5 args? You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > Thanks > > Roberto > > > Roberto > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++ > > > kernel/bpf/btf.c | 39 ++++++++++++++++++++++++------------ > > > net/bpf/test_run.c | 9 +++++++-- > > > 3 files changed, 51 insertions(+), 15 deletions(-) > > > > > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst > > > index c0b7dae6dbf5..41dff6337446 100644 > > > --- a/Documentation/bpf/kfuncs.rst > > > +++ b/Documentation/bpf/kfuncs.rst > > > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz > > annotation, > > > the size of the type > > > of the pointer is used. Without __sz annotation, a kfunc cannot accept a void > > > pointer. > > > > > > +2.2.2 __ref Annotation > > > +---------------------- > > > + > > > +This annotation is used to indicate that the argument is trusted, i.e. it will > > > +be a pointer from an acquire function (defined later), and its offset will be > > > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc > > flag > > > but > > > +only on the parameter it is applied to. An example is shown below:: > > > + > > > + void bpf_task_send_signal(struct task_struct *task__ref, int signal) > > > + { > > > + ... > > > + } > > > + > > > +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and > > > +cannot be used on pointers obtained using pointer walking. This ensures that > > > +caller always calls this kfunc on a task whose lifetime is guaranteed for the > > > +duration of the call. > > > + > > > .. _BPF_kfunc_nodef: > > > > > > 2.3 Using an existing kernel function > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > index 7ac971ea98d1..3ce9b2deef9c 100644 > > > --- a/kernel/bpf/btf.c > > > +++ b/kernel/bpf/btf.c > > > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct > > > bpf_verifier_log *log, > > > return true; > > > } > > > > > > -static bool is_kfunc_arg_mem_size(const struct btf *btf, > > > - const struct btf_param *arg, > > > - const struct bpf_reg_state *reg) > > > +static bool btf_param_match_suffix(const struct btf *btf, > > > + const struct btf_param *arg, > > > + const char *suffix) > > > { > > > - int len, sfx_len = sizeof("__sz") - 1; > > > - const struct btf_type *t; > > > + int len, sfx_len = strlen(suffix); > > > const char *param_name; > > > > > > - t = btf_type_skip_modifiers(btf, arg->type, NULL); > > > - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > > - return false; > > > - > > > /* In the future, this can be ported to use BTF tagging */ > > > param_name = btf_name_by_offset(btf, arg->name_off); > > > if (str_is_empty(param_name)) > > > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct > > btf > > > *btf, > > > if (len < sfx_len) > > > return false; > > > param_name += len - sfx_len; > > > - if (strncmp(param_name, "__sz", sfx_len)) > > > + return !strncmp(param_name, suffix, sfx_len); > > > +} > > > + > > > +static bool is_kfunc_arg_ref(const struct btf *btf, > > > + const struct btf_param *arg) > > > +{ > > > + return btf_param_match_suffix(btf, arg, "__ref"); > > > +} > > > + > > > +static bool is_kfunc_arg_mem_size(const struct btf *btf, > > > + const struct btf_param *arg, > > > + const struct bpf_reg_state *reg) > > > +{ > > > + const struct btf_type *t; > > > + > > > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > > > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > > return false; > > > > > > - return true; > > > + return btf_param_match_suffix(btf, arg, "__sz"); > > > } > > > > > > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct > > > bpf_verifier_env *env, > > > u32 kfunc_flags) > > > { > > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > > - bool rel = false, kptr_get = false, trusted_arg = false; > > > + bool rel = false, kptr_get = false, kf_trusted_args = false; > > > struct bpf_verifier_log *log = &env->log; > > > u32 i, nargs, ref_id, ref_obj_id = 0; > > > bool is_kfunc = btf_is_kernel(btf); > > > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct > > > bpf_verifier_env *env, > > > /* Only kfunc can be release func */ > > > rel = kfunc_flags & KF_RELEASE; > > > kptr_get = kfunc_flags & KF_KPTR_GET; > > > - trusted_arg = kfunc_flags & KF_TRUSTED_ARGS; > > > + kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS; > > > } > > > > > > /* check that BTF function arguments match actual types that the > > > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct > > > bpf_verifier_env *env, > > > enum bpf_arg_type arg_type = ARG_DONTCARE; > > > u32 regno = i + 1; > > > struct bpf_reg_state *reg = ®s[regno]; > > > + bool trusted_arg = false; > > > > > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > > > if (btf_type_is_scalar(t)) { > > > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct > > > bpf_verifier_env *env, > > > /* Check if argument must be a referenced pointer, args + i has > > > * been verified to be a pointer (after skipping modifiers). > > > */ > > > + trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i); > > > if (is_kfunc && trusted_arg && !reg->ref_obj_id) { > > > bpf_log(log, "R%d must be referenced\n", regno); > > > return -EINVAL; > > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > > index cbc9cd5058cb..247bfe52e585 100644 > > > --- a/net/bpf/test_run.c > > > +++ b/net/bpf/test_run.c > > > @@ -691,7 +691,11 @@ noinline void > > bpf_kfunc_call_test_mem_len_fail2(u64 > > > *mem, int len) > > > { > > > } > > > > > > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p) > > > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p) > > > +{ > > > +} > > > + > > > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) > > > { > > > } > > > > > > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3) > > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1) > > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1) > > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2) > > > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS) > > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS) > > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref) > > > BTF_SET8_END(test_sk_check_kfunc_ids) > > > > > > static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > > > -- > > > 2.34.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-28 8:45 ` Kumar Kartikeya Dwivedi @ 2022-07-28 9:02 ` Kumar Kartikeya Dwivedi 2022-08-02 4:45 ` Alexei Starovoitov 2022-07-28 9:02 ` Roberto Sassu 1 sibling, 1 reply; 13+ messages in thread From: Kumar Kartikeya Dwivedi @ 2022-07-28 9:02 UTC (permalink / raw) To: Roberto Sassu; +Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > treat __ref suffix on argument name to imply that it must be a trusted > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag > > > > but limited to the specific parameter. This is required to ensure that > > > > kfunc that operate on some object only work on acquired pointers and not > > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer > > > > walking. Release functions need not specify such suffix on release > > > > arguments as they are already expected to receive one referenced > > > > argument. > > > > > > Thanks, Kumar. I will try it. > > > > Uhm. I realized that I was already using another suffix, > > __maybe_null, to indicate that a caller can pass NULL as > > argument. > > > > Wouldn't probably work well with two suffixes. > > > > Then you can maybe extend it to parse two suffixes at most (for now atleast)? > > > Have you considered to extend BTF_ID_FLAGS to take five > > extra arguments, to set flags for each kfunc parameter? > > > > I didn't understand this. Flags parameter is an OR of the flags you > set, why would we want to extend it to take 5 args? > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. Oh, so you mean having 5 more args to indicate flags on each parameter? It is possible, but I think the scheme for now works ok. If you extend it to parse two suffixes, it should be fine. Yes, the variable name would be ugly, but you can just make a copy into a properly named one. This is the best we can do without switching to BTF tags. We can revisit this when we start having 4 or 5 tags on a single parameter. To make it a bit less verbose you could probably call maybe_null just null? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-28 9:02 ` Kumar Kartikeya Dwivedi @ 2022-08-02 4:45 ` Alexei Starovoitov 2022-08-02 10:07 ` Roberto Sassu 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2022-08-02 4:45 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi Cc: Roberto Sassu, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > > treat __ref suffix on argument name to imply that it must be a trusted > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag > > > > > but limited to the specific parameter. This is required to ensure that > > > > > kfunc that operate on some object only work on acquired pointers and not > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer > > > > > walking. Release functions need not specify such suffix on release > > > > > arguments as they are already expected to receive one referenced > > > > > argument. > > > > > > > > Thanks, Kumar. I will try it. > > > > > > Uhm. I realized that I was already using another suffix, > > > __maybe_null, to indicate that a caller can pass NULL as > > > argument. > > > > > > Wouldn't probably work well with two suffixes. > > > > > > > Then you can maybe extend it to parse two suffixes at most (for now atleast)? > > > > > Have you considered to extend BTF_ID_FLAGS to take five > > > extra arguments, to set flags for each kfunc parameter? > > > > > > > I didn't understand this. Flags parameter is an OR of the flags you > > set, why would we want to extend it to take 5 args? > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > > Oh, so you mean having 5 more args to indicate flags on each > parameter? It is possible, but I think the scheme for now works ok. If > you extend it to parse two suffixes, it should be fine. Yes, the > variable name would be ugly, but you can just make a copy into a > properly named one. This is the best we can do without switching to > BTF tags. We can revisit this when we start having 4 or 5 tags on a > single parameter. > > To make it a bit less verbose you could probably call maybe_null just null? Thank you for posting the patch. It still feels that this extra flexibility gets convoluted. I'm not sure Roberto's kfunc actually needs __ref. All pointers should be pointers. Hacking -1 and -2 into a pointer is something that key infra did, but it doesn't mean that we have to carry over it into bpf kfunc. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-08-02 4:45 ` Alexei Starovoitov @ 2022-08-02 10:07 ` Roberto Sassu 2022-08-02 15:05 ` Alexei Starovoitov 0 siblings, 1 reply; 13+ messages in thread From: Roberto Sassu @ 2022-08-02 10:07 UTC (permalink / raw) To: Alexei Starovoitov, Kumar Kartikeya Dwivedi Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > Sent: Tuesday, August 2, 2022 6:46 AM > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> > wrote: > > > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > > > treat __ref suffix on argument name to imply that it must be a trusted > > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS > flag > > > > > > but limited to the specific parameter. This is required to ensure that > > > > > > kfunc that operate on some object only work on acquired pointers and > not > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by > pointer > > > > > > walking. Release functions need not specify such suffix on release > > > > > > arguments as they are already expected to receive one referenced > > > > > > argument. > > > > > > > > > > Thanks, Kumar. I will try it. > > > > > > > > Uhm. I realized that I was already using another suffix, > > > > __maybe_null, to indicate that a caller can pass NULL as > > > > argument. > > > > > > > > Wouldn't probably work well with two suffixes. > > > > > > > > > > Then you can maybe extend it to parse two suffixes at most (for now > atleast)? > > > > > > > Have you considered to extend BTF_ID_FLAGS to take five > > > > extra arguments, to set flags for each kfunc parameter? > > > > > > > > > > I didn't understand this. Flags parameter is an OR of the flags you > > > set, why would we want to extend it to take 5 args? > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > > > > Oh, so you mean having 5 more args to indicate flags on each > > parameter? It is possible, but I think the scheme for now works ok. If > > you extend it to parse two suffixes, it should be fine. Yes, the > > variable name would be ugly, but you can just make a copy into a > > properly named one. This is the best we can do without switching to > > BTF tags. We can revisit this when we start having 4 or 5 tags on a > > single parameter. > > > > To make it a bit less verbose you could probably call maybe_null just null? > > Thank you for posting the patch. > It still feels that this extra flexibility gets convoluted. > I'm not sure Roberto's kfunc actually needs __ref. > All pointers should be pointers. Hacking -1 and -2 into a pointer > is something that key infra did, but it doesn't mean that > we have to carry over it into bpf kfunc. There is a separate parameter for the keyring IDs that only verify_pkcs7_signature() understands. Type casting is done internally in the bpf_verify_pkcs7_signature() kfunc. The other is always a valid struct key pointer or NULL, coming from bpf_lookup_user_key() (acquire function). I extended Kumar's patch further to annotate the struct key parameter with the _ref_null suffix, to accept a referenced pointer or NULL, instead of just referenced. Roberto ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-08-02 10:07 ` Roberto Sassu @ 2022-08-02 15:05 ` Alexei Starovoitov 2022-08-02 16:01 ` Roberto Sassu 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2022-08-02 15:05 UTC (permalink / raw) To: Roberto Sassu Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > Sent: Tuesday, August 2, 2022 6:46 AM > > > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> > > wrote: > > > > > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> > > wrote: > > > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > > > > treat __ref suffix on argument name to imply that it must be a trusted > > > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS > > flag > > > > > > > but limited to the specific parameter. This is required to ensure that > > > > > > > kfunc that operate on some object only work on acquired pointers and > > not > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by > > pointer > > > > > > > walking. Release functions need not specify such suffix on release > > > > > > > arguments as they are already expected to receive one referenced > > > > > > > argument. > > > > > > > > > > > > Thanks, Kumar. I will try it. > > > > > > > > > > Uhm. I realized that I was already using another suffix, > > > > > __maybe_null, to indicate that a caller can pass NULL as > > > > > argument. > > > > > > > > > > Wouldn't probably work well with two suffixes. > > > > > > > > > > > > > Then you can maybe extend it to parse two suffixes at most (for now > > atleast)? > > > > > > > > > Have you considered to extend BTF_ID_FLAGS to take five > > > > > extra arguments, to set flags for each kfunc parameter? > > > > > > > > > > > > > I didn't understand this. Flags parameter is an OR of the flags you > > > > set, why would we want to extend it to take 5 args? > > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > > > > > > Oh, so you mean having 5 more args to indicate flags on each > > > parameter? It is possible, but I think the scheme for now works ok. If > > > you extend it to parse two suffixes, it should be fine. Yes, the > > > variable name would be ugly, but you can just make a copy into a > > > properly named one. This is the best we can do without switching to > > > BTF tags. We can revisit this when we start having 4 or 5 tags on a > > > single parameter. > > > > > > To make it a bit less verbose you could probably call maybe_null just null? > > > > Thank you for posting the patch. > > It still feels that this extra flexibility gets convoluted. > > I'm not sure Roberto's kfunc actually needs __ref. > > All pointers should be pointers. Hacking -1 and -2 into a pointer > > is something that key infra did, but it doesn't mean that > > we have to carry over it into bpf kfunc. > > There is a separate parameter for the keyring IDs that only > verify_pkcs7_signature() understands. Type casting is done > internally in the bpf_verify_pkcs7_signature() kfunc. > > The other is always a valid struct key pointer or NULL, coming > from bpf_lookup_user_key() (acquire function). I extended > Kumar's patch further to annotate the struct key parameter > with the _ref_null suffix, to accept a referenced pointer or NULL, > instead of just referenced. I don't think it's a good tradeoff complexity wise. !=null check can be done in runtime by the helper. The type cast is a sign of something fishy in the design. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-08-02 15:05 ` Alexei Starovoitov @ 2022-08-02 16:01 ` Roberto Sassu 2022-08-03 14:34 ` Roberto Sassu 0 siblings, 1 reply; 13+ messages in thread From: Roberto Sassu @ 2022-08-02 16:01 UTC (permalink / raw) To: Alexei Starovoitov Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > Sent: Tuesday, August 2, 2022 5:06 PM > On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > Sent: Tuesday, August 2, 2022 6:46 AM > > > > > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi > > > <memxor@gmail.com> wrote: > > > > > > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi > <memxor@gmail.com> > > > wrote: > > > > > > > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu > <roberto.sassu@huawei.com> > > > wrote: > > > > > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > > > > > treat __ref suffix on argument name to imply that it must be a > trusted > > > > > > > > arg when passed to kfunc, similar to the effect of > KF_TRUSTED_ARGS > > > flag > > > > > > > > but limited to the specific parameter. This is required to ensure that > > > > > > > > kfunc that operate on some object only work on acquired pointers > and > > > not > > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by > > > pointer > > > > > > > > walking. Release functions need not specify such suffix on release > > > > > > > > arguments as they are already expected to receive one referenced > > > > > > > > argument. > > > > > > > > > > > > > > Thanks, Kumar. I will try it. > > > > > > > > > > > > Uhm. I realized that I was already using another suffix, > > > > > > __maybe_null, to indicate that a caller can pass NULL as > > > > > > argument. > > > > > > > > > > > > Wouldn't probably work well with two suffixes. > > > > > > > > > > > > > > > > Then you can maybe extend it to parse two suffixes at most (for now > > > atleast)? > > > > > > > > > > > Have you considered to extend BTF_ID_FLAGS to take five > > > > > > extra arguments, to set flags for each kfunc parameter? > > > > > > > > > > > > > > > > I didn't understand this. Flags parameter is an OR of the flags you > > > > > set, why would we want to extend it to take 5 args? > > > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > > > > > > > > Oh, so you mean having 5 more args to indicate flags on each > > > > parameter? It is possible, but I think the scheme for now works ok. If > > > > you extend it to parse two suffixes, it should be fine. Yes, the > > > > variable name would be ugly, but you can just make a copy into a > > > > properly named one. This is the best we can do without switching to > > > > BTF tags. We can revisit this when we start having 4 or 5 tags on a > > > > single parameter. > > > > > > > > To make it a bit less verbose you could probably call maybe_null just null? > > > > > > Thank you for posting the patch. > > > It still feels that this extra flexibility gets convoluted. > > > I'm not sure Roberto's kfunc actually needs __ref. > > > All pointers should be pointers. Hacking -1 and -2 into a pointer > > > is something that key infra did, but it doesn't mean that > > > we have to carry over it into bpf kfunc. > > > > There is a separate parameter for the keyring IDs that only > > verify_pkcs7_signature() understands. Type casting is done > > internally in the bpf_verify_pkcs7_signature() kfunc. > > > > The other is always a valid struct key pointer or NULL, coming > > from bpf_lookup_user_key() (acquire function). I extended > > Kumar's patch further to annotate the struct key parameter > > with the _ref_null suffix, to accept a referenced pointer or NULL, > > instead of just referenced. > > I don't think it's a good tradeoff complexity wise. > !=null check can be done in runtime by the helper. Not sure I follow. bpf_lookup_user_key() might not find the key you asked for, so it will return NULL. Did you mean that I should not set KF_RET_NULL for bpf_lookup_user_key()? That anyway won’t help if you use the system keyring, the alternative parameter. The user keyring is the other one, returned by bpf_lookup_user_key(). When you specify a system keyring, the user keyring should be NULL, to indicate that the parameter should not be used. This was suggested by both John and Daniel. So, it seems unavoidable to annotate the keyring parameter with __ref_null, or at least with __null. __ref_null would be better, to require a referenced parameter. > The type cast is a sign of something fishy in the design. Since this is what verify_pkcs7_signature() accepts, I guess it is the only option. Roberto ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-08-02 16:01 ` Roberto Sassu @ 2022-08-03 14:34 ` Roberto Sassu 0 siblings, 0 replies; 13+ messages in thread From: Roberto Sassu @ 2022-08-03 14:34 UTC (permalink / raw) To: Roberto Sassu, Alexei Starovoitov Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > Sent: Tuesday, August 2, 2022 6:01 PM > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > Sent: Tuesday, August 2, 2022 5:06 PM > > On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com> > > wrote: > > > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > > Sent: Tuesday, August 2, 2022 6:46 AM > > > > > > > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi > > > > <memxor@gmail.com> wrote: > > > > > > > > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi > > <memxor@gmail.com> > > > > wrote: > > > > > > > > > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu > > <roberto.sassu@huawei.com> > > > > wrote: > > > > > > > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > > > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier > to > > > > > > > > > treat __ref suffix on argument name to imply that it must be a > > trusted > > > > > > > > > arg when passed to kfunc, similar to the effect of > > KF_TRUSTED_ARGS > > > > flag > > > > > > > > > but limited to the specific parameter. This is required to ensure > that > > > > > > > > > kfunc that operate on some object only work on acquired pointers > > and > > > > not > > > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by > > > > pointer > > > > > > > > > walking. Release functions need not specify such suffix on release > > > > > > > > > arguments as they are already expected to receive one referenced > > > > > > > > > argument. > > > > > > > > > > > > > > > > Thanks, Kumar. I will try it. > > > > > > > > > > > > > > Uhm. I realized that I was already using another suffix, > > > > > > > __maybe_null, to indicate that a caller can pass NULL as > > > > > > > argument. > > > > > > > > > > > > > > Wouldn't probably work well with two suffixes. > > > > > > > > > > > > > > > > > > > Then you can maybe extend it to parse two suffixes at most (for now > > > > atleast)? > > > > > > > > > > > > > Have you considered to extend BTF_ID_FLAGS to take five > > > > > > > extra arguments, to set flags for each kfunc parameter? > > > > > > > > > > > > > > > > > > > I didn't understand this. Flags parameter is an OR of the flags you > > > > > > set, why would we want to extend it to take 5 args? > > > > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > > > > > > > > > > Oh, so you mean having 5 more args to indicate flags on each > > > > > parameter? It is possible, but I think the scheme for now works ok. If > > > > > you extend it to parse two suffixes, it should be fine. Yes, the > > > > > variable name would be ugly, but you can just make a copy into a > > > > > properly named one. This is the best we can do without switching to > > > > > BTF tags. We can revisit this when we start having 4 or 5 tags on a > > > > > single parameter. > > > > > > > > > > To make it a bit less verbose you could probably call maybe_null just null? > > > > > > > > Thank you for posting the patch. > > > > It still feels that this extra flexibility gets convoluted. > > > > I'm not sure Roberto's kfunc actually needs __ref. > > > > All pointers should be pointers. Hacking -1 and -2 into a pointer > > > > is something that key infra did, but it doesn't mean that > > > > we have to carry over it into bpf kfunc. > > > > > > There is a separate parameter for the keyring IDs that only > > > verify_pkcs7_signature() understands. Type casting is done > > > internally in the bpf_verify_pkcs7_signature() kfunc. > > > > > > The other is always a valid struct key pointer or NULL, coming > > > from bpf_lookup_user_key() (acquire function). I extended > > > Kumar's patch further to annotate the struct key parameter > > > with the _ref_null suffix, to accept a referenced pointer or NULL, > > > instead of just referenced. > > > > I don't think it's a good tradeoff complexity wise. > > !=null check can be done in runtime by the helper. > > Not sure I follow. bpf_lookup_user_key() might not find > the key you asked for, so it will return NULL. > > Did you mean that I should not set KF_RET_NULL for > bpf_lookup_user_key()? > > That anyway won’t help if you use the system keyring, > the alternative parameter. The user keyring is the other > one, returned by bpf_lookup_user_key(). > > When you specify a system keyring, the user keyring should > be NULL, to indicate that the parameter should not be > used. This was suggested by both John and Daniel. > > So, it seems unavoidable to annotate the keyring parameter > with __ref_null, or at least with __null. __ref_null would be > better, to require a referenced parameter. > > > The type cast is a sign of something fishy in the design. > > Since this is what verify_pkcs7_signature() accepts, I guess > it is the only option. I added this topic for the agenda of BPF office hours tomorrow. Roberto ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args 2022-07-28 8:45 ` Kumar Kartikeya Dwivedi 2022-07-28 9:02 ` Kumar Kartikeya Dwivedi @ 2022-07-28 9:02 ` Roberto Sassu 1 sibling, 0 replies; 13+ messages in thread From: Roberto Sassu @ 2022-07-28 9:02 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > Sent: Thursday, July 28, 2022 10:45 AM > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com] > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > treat __ref suffix on argument name to imply that it must be a trusted > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag > > > > but limited to the specific parameter. This is required to ensure that > > > > kfunc that operate on some object only work on acquired pointers and not > > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer > > > > walking. Release functions need not specify such suffix on release > > > > arguments as they are already expected to receive one referenced > > > > argument. > > > > > > Thanks, Kumar. I will try it. > > > > Uhm. I realized that I was already using another suffix, > > __maybe_null, to indicate that a caller can pass NULL as > > argument. > > > > Wouldn't probably work well with two suffixes. > > > > Then you can maybe extend it to parse two suffixes at most (for now atleast)? > > > Have you considered to extend BTF_ID_FLAGS to take five > > extra arguments, to set flags for each kfunc parameter? > > > > I didn't understand this. Flags parameter is an OR of the flags you > set, why would we want to extend it to take 5 args? > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. Instead of using the suffix as mechanism to set per-parameter flags, I would do: BTF_ID_FLAGS(func, bpf_verify_pkcs7_signature, KF_SLEEPABLE, 0, 0, KF_REF | KF_MAYBE_NULL, 0, 0) where the first argument after the kfunc name is used for function-wide flags, the second for first parameter flags, ... Roberto > > Thanks > > > > Roberto > > > > > Roberto > > > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com> > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > --- > > > > Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++ > > > > kernel/bpf/btf.c | 39 ++++++++++++++++++++++++------------ > > > > net/bpf/test_run.c | 9 +++++++-- > > > > 3 files changed, 51 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst > > > > index c0b7dae6dbf5..41dff6337446 100644 > > > > --- a/Documentation/bpf/kfuncs.rst > > > > +++ b/Documentation/bpf/kfuncs.rst > > > > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz > > > annotation, > > > > the size of the type > > > > of the pointer is used. Without __sz annotation, a kfunc cannot accept a > void > > > > pointer. > > > > > > > > +2.2.2 __ref Annotation > > > > +---------------------- > > > > + > > > > +This annotation is used to indicate that the argument is trusted, i.e. it will > > > > +be a pointer from an acquire function (defined later), and its offset will be > > > > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS > kfunc > > > flag > > > > but > > > > +only on the parameter it is applied to. An example is shown below:: > > > > + > > > > + void bpf_task_send_signal(struct task_struct *task__ref, int signal) > > > > + { > > > > + ... > > > > + } > > > > + > > > > +Here, bpf_task_send_signal will only act on trusted task_struct pointers, > and > > > > +cannot be used on pointers obtained using pointer walking. This ensures > that > > > > +caller always calls this kfunc on a task whose lifetime is guaranteed for > the > > > > +duration of the call. > > > > + > > > > .. _BPF_kfunc_nodef: > > > > > > > > 2.3 Using an existing kernel function > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > > index 7ac971ea98d1..3ce9b2deef9c 100644 > > > > --- a/kernel/bpf/btf.c > > > > +++ b/kernel/bpf/btf.c > > > > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct > > > > bpf_verifier_log *log, > > > > return true; > > > > } > > > > > > > > -static bool is_kfunc_arg_mem_size(const struct btf *btf, > > > > - const struct btf_param *arg, > > > > - const struct bpf_reg_state *reg) > > > > +static bool btf_param_match_suffix(const struct btf *btf, > > > > + const struct btf_param *arg, > > > > + const char *suffix) > > > > { > > > > - int len, sfx_len = sizeof("__sz") - 1; > > > > - const struct btf_type *t; > > > > + int len, sfx_len = strlen(suffix); > > > > const char *param_name; > > > > > > > > - t = btf_type_skip_modifiers(btf, arg->type, NULL); > > > > - if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > > > - return false; > > > > - > > > > /* In the future, this can be ported to use BTF tagging */ > > > > param_name = btf_name_by_offset(btf, arg->name_off); > > > > if (str_is_empty(param_name)) > > > > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const > struct > > > btf > > > > *btf, > > > > if (len < sfx_len) > > > > return false; > > > > param_name += len - sfx_len; > > > > - if (strncmp(param_name, "__sz", sfx_len)) > > > > + return !strncmp(param_name, suffix, sfx_len); > > > > +} > > > > + > > > > +static bool is_kfunc_arg_ref(const struct btf *btf, > > > > + const struct btf_param *arg) > > > > +{ > > > > + return btf_param_match_suffix(btf, arg, "__ref"); > > > > +} > > > > + > > > > +static bool is_kfunc_arg_mem_size(const struct btf *btf, > > > > + const struct btf_param *arg, > > > > + const struct bpf_reg_state *reg) > > > > +{ > > > > + const struct btf_type *t; > > > > + > > > > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > > > > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > > > return false; > > > > > > > > - return true; > > > > + return btf_param_match_suffix(btf, arg, "__sz"); > > > > } > > > > > > > > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct > > > > bpf_verifier_env *env, > > > > u32 kfunc_flags) > > > > { > > > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > > > - bool rel = false, kptr_get = false, trusted_arg = false; > > > > + bool rel = false, kptr_get = false, kf_trusted_args = false; > > > > struct bpf_verifier_log *log = &env->log; > > > > u32 i, nargs, ref_id, ref_obj_id = 0; > > > > bool is_kfunc = btf_is_kernel(btf); > > > > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct > > > > bpf_verifier_env *env, > > > > /* Only kfunc can be release func */ > > > > rel = kfunc_flags & KF_RELEASE; > > > > kptr_get = kfunc_flags & KF_KPTR_GET; > > > > - trusted_arg = kfunc_flags & KF_TRUSTED_ARGS; > > > > + kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS; > > > > } > > > > > > > > /* check that BTF function arguments match actual types that the > > > > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct > > > > bpf_verifier_env *env, > > > > enum bpf_arg_type arg_type = ARG_DONTCARE; > > > > u32 regno = i + 1; > > > > struct bpf_reg_state *reg = ®s[regno]; > > > > + bool trusted_arg = false; > > > > > > > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > > > > if (btf_type_is_scalar(t)) { > > > > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct > > > > bpf_verifier_env *env, > > > > /* Check if argument must be a referenced pointer, args + i has > > > > * been verified to be a pointer (after skipping modifiers). > > > > */ > > > > + trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i); > > > > if (is_kfunc && trusted_arg && !reg->ref_obj_id) { > > > > bpf_log(log, "R%d must be referenced\n", regno); > > > > return -EINVAL; > > > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > > > index cbc9cd5058cb..247bfe52e585 100644 > > > > --- a/net/bpf/test_run.c > > > > +++ b/net/bpf/test_run.c > > > > @@ -691,7 +691,11 @@ noinline void > > > bpf_kfunc_call_test_mem_len_fail2(u64 > > > > *mem, int len) > > > > { > > > > } > > > > > > > > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p) > > > > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p) > > > > +{ > > > > +} > > > > + > > > > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) > > > > { > > > > } > > > > > > > > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3) > > > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1) > > > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1) > > > > BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2) > > > > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS) > > > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS) > > > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref) > > > > BTF_SET8_END(test_sk_check_kfunc_ids) > > > > > > > > static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > > > > -- > > > > 2.34.1 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next v1 2/2] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation 2022-07-27 8:15 [PATCH bpf-next v1 0/2] Add __ref annotation for kfuncs Kumar Kartikeya Dwivedi 2022-07-27 8:15 ` [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args Kumar Kartikeya Dwivedi @ 2022-07-27 8:15 ` Kumar Kartikeya Dwivedi 1 sibling, 0 replies; 13+ messages in thread From: Kumar Kartikeya Dwivedi @ 2022-07-27 8:15 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Extend the existing test for KF_TRUSTED_ARGS by also checking whether the same happens when a __ref suffix is present in argument name of a kfunc. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c index 3fb4f69b1962..891fcda50d9d 100644 --- a/tools/testing/selftests/bpf/verifier/calls.c +++ b/tools/testing/selftests/bpf/verifier/calls.c @@ -219,7 +219,7 @@ .errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed", }, { - "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID", + "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (KF_TRUSTED_ARGS)", .insns = { BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8), @@ -227,10 +227,30 @@ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1), BPF_EXIT_INSN(), - BPF_MOV64_REG(BPF_REG_6, BPF_REG_0), - BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), + BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16), BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), - BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, 16), + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_EXIT_INSN(), + }, + .prog_type = BPF_PROG_TYPE_SCHED_CLS, + .fixup_kfunc_btf_id = { + { "bpf_kfunc_call_test_acquire", 3 }, + { "bpf_kfunc_call_test_trusted", 7 }, + }, + .result_unpriv = REJECT, + .result = REJECT, + .errstr = "R1 must be referenced", +}, +{ + "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (__ref)", + .insns = { + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8), + BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), + BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1), + BPF_EXIT_INSN(), + BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16), BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), BPF_MOV64_IMM(BPF_REG_0, 0), BPF_EXIT_INSN(), @@ -238,8 +258,7 @@ .prog_type = BPF_PROG_TYPE_SCHED_CLS, .fixup_kfunc_btf_id = { { "bpf_kfunc_call_test_acquire", 3 }, - { "bpf_kfunc_call_test_ref", 8 }, - { "bpf_kfunc_call_test_ref", 10 }, + { "bpf_kfunc_call_test_ref", 7 }, }, .result_unpriv = REJECT, .result = REJECT, @@ -259,14 +278,17 @@ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0), BPF_MOV64_IMM(BPF_REG_0, 0), BPF_EXIT_INSN(), }, .prog_type = BPF_PROG_TYPE_SCHED_CLS, .fixup_kfunc_btf_id = { { "bpf_kfunc_call_test_acquire", 3 }, - { "bpf_kfunc_call_test_ref", 8 }, - { "bpf_kfunc_call_test_release", 10 }, + { "bpf_kfunc_call_test_trusted", 8 }, + { "bpf_kfunc_call_test_ref", 10 }, + { "bpf_kfunc_call_test_release", 12 }, }, .result_unpriv = REJECT, .result = ACCEPT, -- 2.34.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-08-03 14:35 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-27 8:15 [PATCH bpf-next v1 0/2] Add __ref annotation for kfuncs Kumar Kartikeya Dwivedi 2022-07-27 8:15 ` [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args Kumar Kartikeya Dwivedi 2022-07-28 7:46 ` Roberto Sassu 2022-07-28 8:18 ` Roberto Sassu 2022-07-28 8:45 ` Kumar Kartikeya Dwivedi 2022-07-28 9:02 ` Kumar Kartikeya Dwivedi 2022-08-02 4:45 ` Alexei Starovoitov 2022-08-02 10:07 ` Roberto Sassu 2022-08-02 15:05 ` Alexei Starovoitov 2022-08-02 16:01 ` Roberto Sassu 2022-08-03 14:34 ` Roberto Sassu 2022-07-28 9:02 ` Roberto Sassu 2022-07-27 8:15 ` [PATCH bpf-next v1 2/2] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation Kumar Kartikeya Dwivedi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.