Re: [PATCH v2 bpf-next 1/4] bpf: Add support for per-parameter trusted args

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 5, 2022 at 6:15 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
>
> From: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
>
> 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.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>

LGTM:

Acked-by: Song Liu <song@xxxxxxxxxx>

PS: IIUC, we use "refcounted" and "trusted" for the same concept. Shall we
just use __trusted? Pardon me if this has been discussed before.

Thanks,
Song

> ---
>  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 781731749e55..a9d77d12fd0c 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 903719b89238..7e273f949ee8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6140,18 +6140,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))
> @@ -6160,10 +6155,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,
> @@ -6173,7 +6184,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;
>         bool sleepable = false;
>         struct bpf_verifier_log *log = &env->log;
>         u32 i, nargs, ref_id, ref_obj_id = 0;
> @@ -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;
>                 sleepable = kfunc_flags & KF_SLEEPABLE;
>         }
>
> @@ -6222,6 +6233,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 = &regs[regno];
> +               bool trusted_arg = false;
>
>                 t = btf_type_skip_modifiers(btf, args[i].type, NULL);
>                 if (btf_type_is_scalar(t)) {
> @@ -6240,6 +6252,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 25d8ecf105aa..b735accf8750 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)
>  {
>  }
>
> @@ -722,7 +726,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_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
>  BTF_SET8_END(test_sk_check_kfunc_ids)
>
> --
> 2.37.3
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux