Re: [PATCH v9 bpf-next 08/14] bpf: Add btf_struct_ids_match function

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sat, Aug 1, 2020 at 10:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding btf_struct_ids_match function to check if given address provided
> by BTF object + offset is also address of another nested BTF object.
>
> This allows to pass an argument to helper, which is defined via parent
> BTF object + offset, like for bpf_d_path (added in following changes):
>
>   SEC("fentry/filp_close")
>   int BPF_PROG(prog_close, struct file *file, void *id)
>   {
>     ...
>     ret = bpf_d_path(&file->f_path, ...
>
> The first bpf_d_path argument is hold by verifier as BTF file object
> plus offset of f_path member.
>
> The btf_struct_ids_match function will walk the struct file object and
> check if there's nested struct path object on the given offset.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h   |  2 ++
>  kernel/bpf/btf.c      | 31 +++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c | 20 +++++++++++++-------
>  3 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 40c5e206ecf2..8206d5e324be 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1337,6 +1337,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
>                       const struct btf_type *t, int off, int size,
>                       enum bpf_access_type atype,
>                       u32 *next_btf_id);
> +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> +                         int off, u32 id, u32 need_type_id);
>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>                           const struct bpf_func_proto *fn, int);
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 7bacc2f56061..ba05b15ad599 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4160,6 +4160,37 @@ int btf_struct_access(struct bpf_verifier_log *log,
>         return -EINVAL;
>  }
>
> +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> +                         int off, u32 id, u32 need_type_id)
> +{
> +       const struct btf_type *type;
> +       int err;
> +
> +       /* Are we already done? */
> +       if (need_type_id == id && off == 0)
> +               return true;
> +
> +again:
> +       type = btf_type_by_id(btf_vmlinux, id);
> +       if (!type)
> +               return false;
> +       err = btf_struct_walk(log, type, off, 1, &id);

nit: this size=1 looks a bit artificial, seems like btf_struct_walk()
will work with size==0 just as well, no?

> +       if (err != WALK_STRUCT)
> +               return false;
> +
> +       /* We found nested struct object. If it matches
> +        * the requested ID, we're done. Otherwise let's
> +        * continue the search with offset 0 in the new
> +        * type.
> +        */
> +       if (need_type_id != id) {
> +               off = 0;
> +               goto again;
> +       }
> +
> +       return true;
> +}
> +
>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>                           const struct bpf_func_proto *fn, int arg)
>  {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b6ccfce3bf4c..bb6ca19f282d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3960,16 +3960,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                                 goto err_type;
>                 }
>         } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> +               bool ids_match = false;
> +
>                 expected_type = PTR_TO_BTF_ID;
>                 if (type != expected_type)
>                         goto err_type;
>                 if (!fn->check_btf_id) {
> -                       if (reg->btf_id != meta->btf_id) {
> -                               verbose(env, "Helper has type %s got %s in R%d\n",
> -                                       kernel_type_name(meta->btf_id),
> -                                       kernel_type_name(reg->btf_id), regno);
> -
> -                               return -EACCES;
> +                       if (reg->btf_id != meta->btf_id || reg->off) {

Will it ever succeed if reg->btf_id == meta->btf_id, but reg->off > 0?
That would require recursively nested type, which is not possible,
right? Or what am I missing? Is it just a simplification of the error
handling path?

> +                               ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> +                                                                meta->btf_id);
> +                               if (!ids_match) {
> +                                       verbose(env, "Helper has type %s got %s in R%d\n",
> +                                               kernel_type_name(meta->btf_id),
> +                                               kernel_type_name(reg->btf_id), regno);
> +                                       return -EACCES;
> +                               }
>                         }
>                 } else if (!fn->check_btf_id(reg->btf_id, arg)) {
>                         verbose(env, "Helper does not support %s in R%d\n",
> @@ -3977,7 +3982,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>
>                         return -EACCES;
>                 }
> -               if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) {
> +               if (!ids_match &&
> +                   (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) {
>                         verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
>                                 regno);
>                         return -EACCES;
> --
> 2.25.4
>