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:   Fri, 11 Oct 2019 12:02:50 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <ast@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>, x86@...nel.org,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 10/12] bpf: check types of arguments passed
 into helpers

On Wed, Oct 9, 2019 at 9:15 PM Alexei Starovoitov <ast@...nel.org> wrote:
>
> Introduce new helper that reuses existing skb perf_event output
> implementation, but can be called from raw_tracepoint programs
> that receive 'struct sk_buff *' as tracepoint argument or
> can walk other kernel data structures to skb pointer.
>
> In order to do that teach verifier to resolve true C types
> of bpf helpers into in-kernel BTF ids.
> The type of kernel pointer passed by raw tracepoint into bpf
> program will be tracked by the verifier all the way until
> it's passed into helper function.
> For example:
> kfree_skb() kernel function calls trace_kfree_skb(skb, loc);
> bpf programs receives that skb pointer and may eventually
> pass it into bpf_skb_output() bpf helper which in-kernel is
> implemented via bpf_skb_event_output() kernel function.
> Its first argument in the kernel is 'struct sk_buff *'.
> The verifier makes sure that types match all the way.
>
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---
>  include/linux/bpf.h            | 18 ++++++---
>  include/uapi/linux/bpf.h       | 27 +++++++++++++-
>  kernel/bpf/btf.c               | 68 ++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c          | 44 ++++++++++++++--------
>  kernel/trace/bpf_trace.c       |  4 ++
>  net/core/filter.c              | 15 +++++++-
>  tools/include/uapi/linux/bpf.h | 27 +++++++++++++-
>  7 files changed, 180 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6edfe50f1c2c..d3df073f374a 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -213,6 +213,7 @@ enum bpf_arg_type {
>         ARG_PTR_TO_INT,         /* pointer to int */
>         ARG_PTR_TO_LONG,        /* pointer to long */
>         ARG_PTR_TO_SOCKET,      /* pointer to bpf_sock (fullsock) */
> +       ARG_PTR_TO_BTF_ID,      /* pointer to in-kernel struct */
>  };
>
>  /* type of values returned from helper functions */
> @@ -235,11 +236,17 @@ struct bpf_func_proto {
>         bool gpl_only;
>         bool pkt_access;
>         enum bpf_return_type ret_type;
> -       enum bpf_arg_type arg1_type;
> -       enum bpf_arg_type arg2_type;
> -       enum bpf_arg_type arg3_type;
> -       enum bpf_arg_type arg4_type;
> -       enum bpf_arg_type arg5_type;
> +       union {
> +               struct {
> +                       enum bpf_arg_type arg1_type;
> +                       enum bpf_arg_type arg2_type;
> +                       enum bpf_arg_type arg3_type;
> +                       enum bpf_arg_type arg4_type;
> +                       enum bpf_arg_type arg5_type;
> +               };
> +               enum bpf_arg_type arg_type[5];
> +       };
> +       u32 *btf_id; /* BTF ids of arguments */

are you trying to save memory with this? otherwise not sure why it's
not just `u32 btf_id[5]`? Even in that case it will save at most 12
bytes (and I haven't even check alignment padding and stuff). So
doesn't seem worth it?

>  };
>
>  /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
> @@ -765,6 +772,7 @@ 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);
> +u32 btf_resolve_helper_id(struct bpf_verifier_log *log, void *, int);
>
>  #else /* !CONFIG_BPF_SYSCALL */
>  static inline struct bpf_prog *bpf_prog_get(u32 ufd)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3bb2cd1de341..b0454440186f 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2751,6 +2751,30 @@ union bpf_attr {
>   *             **-EOPNOTSUPP** kernel configuration does not enable SYN cookies
>   *
>   *             **-EPROTONOSUPPORT** IP packet version is not 4 or 6
> + *
> + * int bpf_skb_output(void *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
> + *     Description
> + *             Write raw *data* blob into a special BPF perf event held by
> + *             *map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
> + *             event must have the following attributes: **PERF_SAMPLE_RAW**
> + *             as **sample_type**, **PERF_TYPE_SOFTWARE** as **type**, and
> + *             **PERF_COUNT_SW_BPF_OUTPUT** as **config**.
> + *
> + *             The *flags* are used to indicate the index in *map* for which
> + *             the value must be put, masked with **BPF_F_INDEX_MASK**.
> + *             Alternatively, *flags* can be set to **BPF_F_CURRENT_CPU**
> + *             to indicate that the index of the current CPU core should be
> + *             used.
> + *
> + *             The value to write, of *size*, is passed through eBPF stack and
> + *             pointed by *data*.

typo? pointed __to__ by *data*?

> + *
> + *             *ctx* is a pointer to in-kernel sutrct sk_buff.
> + *
> + *             This helper is similar to **bpf_perf_event_output**\ () but
> + *             restricted to raw_tracepoint bpf programs.

nit: with BTF type tracking enabled?

> + *     Return
> + *             0 on success, or a negative error in case of failure.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -2863,7 +2887,8 @@ union bpf_attr {
>         FN(sk_storage_get),             \
>         FN(sk_storage_delete),          \
>         FN(send_signal),                \
> -       FN(tcp_gen_syncookie),
> +       FN(tcp_gen_syncookie),          \
> +       FN(skb_output),
>

[...]

> @@ -4072,21 +4091,16 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>
>         meta.func_id = func_id;
>         /* check args */
> -       err = check_func_arg(env, BPF_REG_1, fn->arg1_type, &meta);
> -       if (err)
> -               return err;
> -       err = check_func_arg(env, BPF_REG_2, fn->arg2_type, &meta);
> -       if (err)
> -               return err;
> -       err = check_func_arg(env, BPF_REG_3, fn->arg3_type, &meta);
> -       if (err)
> -               return err;
> -       err = check_func_arg(env, BPF_REG_4, fn->arg4_type, &meta);
> -       if (err)
> -               return err;
> -       err = check_func_arg(env, BPF_REG_5, fn->arg5_type, &meta);
> -       if (err)
> -               return err;
> +       for (i = 0; i < 5; i++) {
> +               if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID) {
> +                       if (!fn->btf_id[i])
> +                               fn->btf_id[i] = btf_resolve_helper_id(&env->log, fn->func, 0);

bug: 0 -> i  :)



> +                       meta.btf_id = fn->btf_id[i];
> +               }
> +               err = check_func_arg(env, BPF_REG_1 + i, fn->arg_type[i], &meta);
> +               if (err)
> +                       return err;
> +       }
>

[...]

Powered by blists - more mailing lists