netdev - Re: [PATCH v10 bpf-next 08/14] bpf: Add btf_struct_ids

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzY8vE8k9c5fBB+3mcEpxOWc38dWBK8ji2aRpHM79nra_Q@mail.gmail.com>
Date:   Fri, 7 Aug 2020 13:04:26 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v10 bpf-next 08/14] bpf: Add btf_struct_ids_match function

On Fri, Aug 7, 2020 at 2:47 AM Jiri Olsa <jolsa@...nel.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@...nel.org>
> ---
>  include/linux/bpf.h   |  2 ++
>  kernel/bpf/btf.c      | 31 +++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c | 18 ++++++++++++------
>  3 files changed, 45 insertions(+), 6 deletions(-)
>

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b6ccfce3bf4c..041d151be15b 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;
> +                               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)) {

Put this on a wishlist for now. I don't think we should expect
fb->check_btf_id() to do btf_struct_ids_match() internally, so to
support this, we'd have to call fb->check_btf_id() inside the loop
while doing WALK_STRUCT struct. But let's not change all this in this
patch set, it's involved enough already.

>                         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)) {

Isn't this still wrong? if ids_match, but reg->var_off is non-zero,
that's still bad, right?
ids_match just "mitigates" reg->off check, so should be something like this:

if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) ||
reg->var_off.value)
 ... then bad ...

>                         verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
>                                 regno);
>                         return -EACCES;
> --
> 2.25.4
>