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]
Date:   Wed, 16 Sep 2020 10:32:09 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()

On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@...hat.com>
>
> The check_attach_btf_id() function really does three things:
>
> 1. It performs a bunch of checks on the program to ensure that the
>    attachment is valid.
>
> 2. It stores a bunch of state about the attachment being requested in
>    the verifier environment and struct bpf_prog objects.
>
> 3. It allocates a trampoline for the attachment.
>
> This patch splits out (1.) and (3.) into separate functions in preparation
> for reusing them when the actual attachment is happening (in the
> raw_tracepoint_open syscall operation), which will allow tracing programs
> to have multiple (compatible) attachments.
>
> No functional change is intended with this patch.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---

I almost acked this, but found a problem at the very last moment. See
below, along with few more comments while I have enough context in my
head.

BTW, for whatever reason your patches arrived with a 12 hour delay
yesterday (cover letter received at 5am, while patches arrived at
6pm), don't know if its vger or gmail...

>  include/linux/bpf.h          |    7 +
>  include/linux/bpf_verifier.h |    9 ++
>  kernel/bpf/trampoline.c      |   20 ++++
>  kernel/bpf/verifier.c        |  197 ++++++++++++++++++++++++------------------
>  4 files changed, 149 insertions(+), 84 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 5ad4a935a24e..dcf0c70348a4 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -616,6 +616,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
>  struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
>  int bpf_trampoline_link_prog(struct bpf_prog *prog);
>  int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
> +struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
> +                                         struct btf_func_model *fmodel);
>  void bpf_trampoline_put(struct bpf_trampoline *tr);
>  #define BPF_DISPATCHER_INIT(_name) {                           \
>         .mutex = __MUTEX_INITIALIZER(_name.mutex),              \
> @@ -672,6 +674,11 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
>  {
>         return -ENOTSUPP;
>  }
> +static inline struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
> +                                                       struct btf_func_model *fmodel)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
>  static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {}
>  #define DEFINE_BPF_DISPATCHER(name)
>  #define DECLARE_BPF_DISPATCHER(name)
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 20009e766805..db3db0b69aad 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -447,4 +447,13 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
>  int check_ctx_reg(struct bpf_verifier_env *env,
>                   const struct bpf_reg_state *reg, int regno);
>
> +int bpf_check_attach_target(struct bpf_verifier_log *log,
> +                           const struct bpf_prog *prog,
> +                           const struct bpf_prog *tgt_prog,
> +                           u32 btf_id,
> +                           struct btf_func_model *fmodel,
> +                           long *tgt_addr,
> +                           const char **tgt_name,
> +                           const struct btf_type **tgt_type);

So this is obviously an abomination of a function signature,
especially for a one exported to other files.

One candidate to remove would be tgt_type, which is supposed to be a
derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,
**except** (and that's how I found the bug below), in case of
fentry/fexit programs attaching to "conservative" BPF functions, in
which case what's stored in aux->attach_func_proto is different from
what is passed into btf_distill_func_proto. So that's a bug already
(you'll return NULL in some cases for tgt_type, while it has to always
be non-NULL).

But related to that is fmodel. It seems like bpf_check_attach_target()
has no interest in fmodel itself and is just passing it from
btf_distill_func_proto(). So I was about to suggest dropping fmodel
and calling btf_distill_func_proto() outside of
bpf_check_attach_target(), but given the conservative + fentry/fexit
quirk, it's probably going to be more confusing.

So with all this, I suggest dropping the tgt_type output param
altogether and let callers do a `btf__type_by_id(tgt_prog ?
tgt_prog->aux->btf : btf_vmlinux, btf_id);`. That will both fix the
bug and will make this function's signature just a tad bit less
horrible.

> +
>  #endif /* _LINUX_BPF_VERIFIER_H */
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 7dd523a7e32d..7845913e7e41 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -336,6 +336,26 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
>         return err;
>  }
>
> +struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
> +                                         struct btf_func_model *fmodel)
> +{
> +       struct bpf_trampoline *tr;
> +
> +       tr = bpf_trampoline_lookup(key);
> +       if (!tr)
> +               return ERR_PTR(-ENOMEM);

So seems like the only way this function can fail is when
bpf_trampoline_lookup() returns NULL (and we assume -ENOMEM then), so
I guess we could have just returned NULL the same to keep
bpf_trampoline_lookup() and bpf_trampoline_get() similar. But it's
minor, if you prefer to encode error code anyways.

> +
> +       mutex_lock(&tr->mutex);
> +       if (tr->func.addr)
> +               goto out;
> +
> +       memcpy(&tr->func.model, fmodel, sizeof(*fmodel));
> +       tr->func.addr = addr;
> +out:
> +       mutex_unlock(&tr->mutex);
> +       return tr;
> +}
> +
>  void bpf_trampoline_put(struct bpf_trampoline *tr)
>  {
>         if (!tr)

[...]

> @@ -11235,24 +11202,14 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>                 t = btf_type_by_id(btf, t->type);
>                 if (!btf_type_is_func_proto(t))
>                         return -EINVAL;
> -               tr = bpf_trampoline_lookup(key);
> -               if (!tr)
> -                       return -ENOMEM;
> -               /* t is either vmlinux type or another program's type */
> -               prog->aux->attach_func_proto = t;
> -               mutex_lock(&tr->mutex);
> -               if (tr->func.addr) {
> -                       prog->aux->trampoline = tr;
> -                       goto out;
> -               }
> -               if (tgt_prog && conservative) {
> -                       prog->aux->attach_func_proto = NULL;
> +
> +               if (tgt_prog && conservative)
>                         t = NULL;

this is where the bug happens, we can't return this NULL to caller as tgt_prog

> -               }
> -               ret = btf_distill_func_proto(log, btf, t,
> -                                            tname, &tr->func.model);
> +
> +               ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
>                 if (ret < 0)
> -                       goto out;
> +                       return ret;
> +
>                 if (tgt_prog) {
>                         if (subprog == 0)
>                                 addr = (long) tgt_prog->bpf_func;

[...]

Powered by blists - more mailing lists