On Sat, Aug 1, 2020 at 10:04 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
