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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJnrk1Zu7FZLu2sYcOgOejk=FibouSZT=nEFnpYwnZN7TAo7+w@mail.gmail.com>
Date:   Fri, 17 Feb 2023 15:00:30 -0800
From:   Joanne Koong <joannelkoong@...il.com>
To:     bpf@...r.kernel.org
Cc:     martin.lau@...nel.org, andrii@...nel.org, memxor@...il.com,
        ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
        kernel-team@...com
Subject: Re: [PATCH v10 bpf-next 8/9] bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr

On Thu, Feb 16, 2023 at 3:00 PM Joanne Koong <joannelkoong@...il.com> wrote:
>
> Two new kfuncs are added, bpf_dynptr_slice and bpf_dynptr_slice_rdwr.
> The user must pass in a buffer to store the contents of the data slice
> if a direct pointer to the data cannot be obtained.
>
> For skb and xdp type dynptrs, these two APIs are the only way to obtain
> a data slice. However, for other types of dynptrs, there is no
> difference between bpf_dynptr_slice(_rdwr) and bpf_dynptr_data.
>
> For skb type dynptrs, the data is copied into the user provided buffer
> if any of the data is not in the linear portion of the skb. For xdp type
> dynptrs, the data is copied into the user provided buffer if the data is
> between xdp frags.
>
> If the skb is cloned and a call to bpf_dynptr_data_rdwr is made, then
> the skb will be uncloned (see bpf_unclone_prologue()).
>
> Please note that any bpf_dynptr_write() automatically invalidates any prior
> data slices of the skb dynptr. This is because the skb may be cloned or
> may need to pull its paged buffer into the head. As such, any
> bpf_dynptr_write() will automatically have its prior data slices
> invalidated, even if the write is to data in the skb head of an uncloned
> skb. Please note as well that any other helper calls that change the
> underlying packet buffer (eg bpf_skb_pull_data()) invalidates any data
> slices of the skb dynptr as well, for the same reasons.
>
> Signed-off-by: Joanne Koong <joannelkoong@...il.com>
> ---
>  include/linux/filter.h         | 14 ++++++
>  include/uapi/linux/bpf.h       |  5 ++
>  kernel/bpf/helpers.c           | 91 ++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c          | 88 ++++++++++++++++++++++++++++++--
>  net/core/filter.c              |  6 +--
>  tools/include/uapi/linux/bpf.h |  5 ++
>  6 files changed, 202 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 3f6992261ec5..efa5d4a1677e 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1548,6 +1548,9 @@ int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
>                           u32 len, u64 flags);
>  int __bpf_xdp_load_bytes(struct xdp_buff *xdp, u32 offset, void *buf, u32 len);
>  int __bpf_xdp_store_bytes(struct xdp_buff *xdp, u32 offset, void *buf, u32 len);
> +void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len);
> +void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
> +                     void *buf, unsigned long len, bool flush);
>  #else /* CONFIG_NET */
>  static inline int __bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset,
>                                        void *to, u32 len)
> @@ -1572,6 +1575,17 @@ static inline int __bpf_xdp_store_bytes(struct xdp_buff *xdp, u32 offset,
>  {
>         return -EOPNOTSUPP;
>  }
> +
> +static inline void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len)
> +{
> +       return NULL;
> +}
> +
> +static inline void *bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off, void *buf,
> +                                    unsigned long len, bool flush)
> +{
> +       return NULL;
> +}
>  #endif /* CONFIG_NET */
>
>  #endif /* __LINUX_FILTER_H__ */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e7435acbdd30..ac406af207c3 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5324,6 +5324,11 @@ union bpf_attr {
>   *             *flags* must be 0 except for skb-type dynptrs.
>   *
>   *             For skb-type dynptrs:
> + *                 *  All data slices of the dynptr are automatically
> + *                    invalidated after **bpf_dynptr_write**\ (). This is
> + *                    because writing may pull the skb and change the
> + *                    underlying packet buffer.
> + *
>   *                 *  For *flags*, please see the flags accepted by
>   *                    **bpf_skb_store_bytes**\ ().
>   *     Return
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 989be97b0f81..0586f54e4f9e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2177,6 +2177,94 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
>         return p;
>  }
>
> +/**
> + * bpf_dynptr_slice - Obtain a read-only pointer to the dynptr data.
> + *
> + * For non-skb and non-xdp type dynptrs, there is no difference between
> + * bpf_dynptr_slice and bpf_dynptr_data.
> + *
> + * If the intention is to write to the data slice, please use
> + * bpf_dynptr_slice_rdwr.
> + *
> + * @ptr: The dynptr whose data slice to retrieve
> + * @offset: Offset into the dynptr
> + * @buffer: User-provided buffer to copy contents into
> + * @buffer__sz: Size (in bytes) of the buffer. This is the length of the
> + * requested slice
> + *
> + * @returns: NULL if the call failed (eg invalid dynptr), pointer to a read-only
> + * data slice (can be either direct pointer to the data or a pointer to the user
> + * provided buffer, with its contents containing the data, if unable to obtain
> + * direct pointer)
> + */
> +__bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
> +                                  void *buffer, u32 buffer__sz)
> +{
> +       enum bpf_dynptr_type type;
> +       u32 len = buffer__sz;
> +       int err;
> +
> +       if (!ptr->data)
> +               return 0;
> +
> +       err = bpf_dynptr_check_off_len(ptr, offset, len);
> +       if (err)
> +               return 0;
> +
> +       type = bpf_dynptr_get_type(ptr);
> +
> +       switch (type) {
> +       case BPF_DYNPTR_TYPE_LOCAL:
> +       case BPF_DYNPTR_TYPE_RINGBUF:
> +               return ptr->data + ptr->offset + offset;
> +       case BPF_DYNPTR_TYPE_SKB:
> +               return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer);
> +       case BPF_DYNPTR_TYPE_XDP:
> +       {
> +               void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
> +               if (xdp_ptr)
> +                       return xdp_ptr;
> +
> +               bpf_xdp_copy_buf(ptr->data, ptr->offset + offset, buffer, len, false);
> +               return buffer;
> +       }
> +       default:
> +               WARN_ONCE(true, "unknown dynptr type %d\n", type);
> +               return 0;
> +       }
> +}
> +
> +/**
> + * bpf_dynptr_slice_rdwr - Obtain a pointer to the dynptr data.
> + *
> + * For non-skb and non-xdp type dynptrs, there is no difference between
> + * bpf_dynptr_slice and bpf_dynptr_data.
> + *
> + * @ptr: The dynptr whose data slice to retrieve
> + * @offset: Offset into the dynptr
> + * @buffer: User-provided buffer to copy contents into
> + * @buffer__sz: Size (in bytes) of the buffer. This is the length of the
> + * requested slice
> + *
> + * @returns: NULL if the call failed (eg invalid dynptr), pointer to a
> + * data slice (can be either direct pointer to the data or a pointer to the user
> + * provided buffer, with its contents containing the data, if unable to obtain
> + * direct pointer)
> + */
> +__bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
> +                                       void *buffer, u32 buffer__sz)
> +{
> +       if (!ptr->data || bpf_dynptr_is_rdonly(ptr))
> +               return 0;
> +
> +       /* bpf_dynptr_slice_rdwr is the same logic as bpf_dynptr_slice.
> +        *
> +        * For skb-type dynptrs, the verifier has already ensured that the skb
> +        * head is writable (see bpf_unclone_prologue()).
> +        */
> +       return bpf_dynptr_slice(ptr, offset, buffer, buffer__sz);
> +}
> +
>  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
>  {
>         return obj;
> @@ -2224,6 +2312,8 @@ BTF_ID_FLAGS(func, bpf_cgroup_release, KF_RELEASE)
>  BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)
>  #endif
>  BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_dynptr_slice, KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)

upon further thought, I think these two should be listed in the
"common_btf_ids" BTF set, not "generic_btf_ids", since common_btf_ids
is registered for BPF_PROG_TYPE_UNSPEC prog types

>  BTF_SET8_END(generic_btf_ids)
>
>  static const struct btf_kfunc_id_set generic_kfunc_set = {
> @@ -2271,6 +2361,7 @@ static int __init kfunc_init(void)
>         ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &generic_kfunc_set);
>         ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &generic_kfunc_set);
>         ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &generic_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &generic_kfunc_set);
>         ret = ret ?: register_btf_id_dtor_kfuncs(generic_dtors,
>                                                   ARRAY_SIZE(generic_dtors),
>                                                   THIS_MODULE);
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0b767134ecaa..aa8d48b00cc2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -759,6 +759,22 @@ static enum bpf_dynptr_type arg_to_dynptr_type(enum bpf_arg_type arg_type)
>         }
>  }
>
> +static enum bpf_type_flag get_dynptr_type_flag(enum bpf_dynptr_type type)
> +{
> +       switch (type) {
> +       case BPF_DYNPTR_TYPE_LOCAL:
> +               return DYNPTR_TYPE_LOCAL;
> +       case BPF_DYNPTR_TYPE_RINGBUF:
> +               return DYNPTR_TYPE_RINGBUF;
> +       case BPF_DYNPTR_TYPE_SKB:
> +               return DYNPTR_TYPE_SKB;
> +       case BPF_DYNPTR_TYPE_XDP:
> +               return DYNPTR_TYPE_XDP;
> +       default:
> +               return 0;
> +       }
> +}
> +
>  static bool dynptr_type_refcounted(enum bpf_dynptr_type type)
>  {
>         return type == BPF_DYNPTR_TYPE_RINGBUF;
> @@ -1677,6 +1693,12 @@ static bool reg_is_pkt_pointer_any(const struct bpf_reg_state *reg)
>                reg->type == PTR_TO_PACKET_END;
>  }
>
> +static bool reg_is_dynptr_slice_pkt(const struct bpf_reg_state *reg)
> +{
> +       return base_type(reg->type) == PTR_TO_MEM &&
> +               (reg->type & DYNPTR_TYPE_SKB || reg->type & DYNPTR_TYPE_XDP);
> +}
> +
>  /* Unmodified PTR_TO_PACKET[_META,_END] register from ctx access. */
>  static bool reg_is_init_pkt_pointer(const struct bpf_reg_state *reg,
>                                     enum bpf_reg_type which)
> @@ -7404,6 +7426,9 @@ static int check_func_proto(const struct bpf_func_proto *fn, int func_id)
>
>  /* Packet data might have moved, any old PTR_TO_PACKET[_META,_END]
>   * are now invalid, so turn them into unknown SCALAR_VALUE.
> + *
> + * This also applies to dynptr slices belonging to skb and xdp dynptrs,
> + * since these slices point to packet data.
>   */
>  static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
>  {
> @@ -7411,7 +7436,7 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
>         struct bpf_reg_state *reg;
>
>         bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
> -               if (reg_is_pkt_pointer_any(reg))
> +               if (reg_is_pkt_pointer_any(reg) || reg_is_dynptr_slice_pkt(reg))
>                         __mark_reg_unknown(env, reg);
>         }));
>  }
> @@ -8667,6 +8692,11 @@ struct bpf_kfunc_call_arg_meta {
>         struct {
>                 struct btf_field *field;
>         } arg_rbtree_root;
> +       struct {
> +               enum bpf_dynptr_type type;
> +               u32 id;
> +       } initialized_dynptr;
> +       u64 mem_size;
>  };
>
>  static bool is_kfunc_acquire(struct bpf_kfunc_call_arg_meta *meta)
> @@ -8923,6 +8953,8 @@ enum special_kfunc_type {
>         KF_bpf_rbtree_first,
>         KF_bpf_dynptr_from_skb,
>         KF_bpf_dynptr_from_xdp,
> +       KF_bpf_dynptr_slice,
> +       KF_bpf_dynptr_slice_rdwr,
>  };
>
>  BTF_SET_START(special_kfunc_set)
> @@ -8939,6 +8971,8 @@ BTF_ID(func, bpf_rbtree_add)
>  BTF_ID(func, bpf_rbtree_first)
>  BTF_ID(func, bpf_dynptr_from_skb)
>  BTF_ID(func, bpf_dynptr_from_xdp)
> +BTF_ID(func, bpf_dynptr_slice)
> +BTF_ID(func, bpf_dynptr_slice_rdwr)
>  BTF_SET_END(special_kfunc_set)
>
>  BTF_ID_LIST(special_kfunc_list)
> @@ -8957,6 +8991,8 @@ BTF_ID(func, bpf_rbtree_add)
>  BTF_ID(func, bpf_rbtree_first)
>  BTF_ID(func, bpf_dynptr_from_skb)
>  BTF_ID(func, bpf_dynptr_from_xdp)
> +BTF_ID(func, bpf_dynptr_slice)
> +BTF_ID(func, bpf_dynptr_slice_rdwr)
>
>  static bool is_kfunc_bpf_rcu_read_lock(struct bpf_kfunc_call_arg_meta *meta)
>  {
> @@ -9710,12 +9746,24 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>                                 dynptr_arg_type |= MEM_UNINIT | DYNPTR_TYPE_SKB;
>                         else if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_xdp])
>                                 dynptr_arg_type |= MEM_UNINIT | DYNPTR_TYPE_XDP;
> -                       else
> +                       else if (reg->type == CONST_PTR_TO_DYNPTR)
>                                 dynptr_arg_type |= MEM_RDONLY;
>
>                         ret = process_dynptr_func(env, regno, insn_idx, dynptr_arg_type);
>                         if (ret < 0)
>                                 return ret;
> +
> +                       if (!(dynptr_arg_type & MEM_UNINIT)) {
> +                               int id = dynptr_id(env, reg);
> +
> +                               if (id < 0) {
> +                                       verbose(env, "verifier internal error: failed to obtain dynptr id\n");
> +                                       return id;
> +                               }
> +                               meta->initialized_dynptr.id = id;
> +                               meta->initialized_dynptr.type = dynptr_get_type(env, reg);
> +                       }
> +
>                         break;
>                 }
>                 case KF_ARG_PTR_TO_LIST_HEAD:
> @@ -9966,8 +10014,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                 }
>         }
>
> -       for (i = 0; i < CALLER_SAVED_REGS; i++)
> -               mark_reg_not_init(env, regs, caller_saved[i]);
> +       mark_reg_not_init(env, regs, caller_saved[BPF_REG_0]);
>
>         /* Check return type */
>         t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
> @@ -10053,6 +10100,36 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                                 regs[BPF_REG_0].type = PTR_TO_BTF_ID | PTR_UNTRUSTED;
>                                 regs[BPF_REG_0].btf = desc_btf;
>                                 regs[BPF_REG_0].btf_id = meta.arg_constant.value;
> +                       } else if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice] ||
> +                                  meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr]) {
> +                               enum bpf_type_flag type_flag = get_dynptr_type_flag(meta.initialized_dynptr.type);
> +
> +                               mark_reg_known_zero(env, regs, BPF_REG_0);
> +
> +                               if (!tnum_is_const(regs[BPF_REG_4].var_off)) {
> +                                       verbose(env, "mem_size must be a constant\n");
> +                                       return -EINVAL;
> +                               }
> +                               regs[BPF_REG_0].mem_size = regs[BPF_REG_4].var_off.value;
> +
> +                               /* PTR_MAYBE_NULL will be added when is_kfunc_ret_null is checked */
> +                               regs[BPF_REG_0].type = PTR_TO_MEM | type_flag;
> +
> +                               if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice])
> +                                       regs[BPF_REG_0].type |= MEM_RDONLY;
> +                               else
> +                                       env->seen_direct_write = true;
> +
> +                               if (!meta.initialized_dynptr.id) {
> +                                       verbose(env, "verifier internal error: no dynptr id\n");
> +                                       return -EFAULT;
> +                               }
> +                               regs[BPF_REG_0].dynptr_id = meta.initialized_dynptr.id;
> +
> +                               /* we don't need to set BPF_REG_0's ref obj id
> +                                * because packet slices are not refcounted (see
> +                                * dynptr_type_refcounted)
> +                                */
>                         } else {
>                                 verbose(env, "kernel function %s unhandled dynamic return type\n",
>                                         meta.func_name);
> @@ -10112,6 +10189,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                         regs[BPF_REG_0].id = ++env->id_gen;
>         } /* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */
>
> +       for (i = BPF_REG_1; i < CALLER_SAVED_REGS; i++)
> +               mark_reg_not_init(env, regs, caller_saved[i]);
> +
>         nargs = btf_type_vlen(func_proto);
>         args = (const struct btf_param *)(func_proto + 1);
>         for (i = 0; i < nargs; i++) {
> diff --git a/net/core/filter.c b/net/core/filter.c
> index dcf1e6d2582d..0bd62b2b25e1 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3894,8 +3894,8 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
>         .arg2_type      = ARG_ANYTHING,
>  };
>
> -static void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
> -                            void *buf, unsigned long len, bool flush)
> +void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
> +                     void *buf, unsigned long len, bool flush)
>  {
>         unsigned long ptr_len, ptr_off = 0;
>         skb_frag_t *next_frag, *end_frag;
> @@ -3941,7 +3941,7 @@ static void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
>         }
>  }
>
> -static void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len)
> +void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len)
>  {
>         struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>         u32 size = xdp->data_end - xdp->data;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index e7435acbdd30..ac406af207c3 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5324,6 +5324,11 @@ union bpf_attr {
>   *             *flags* must be 0 except for skb-type dynptrs.
>   *
>   *             For skb-type dynptrs:
> + *                 *  All data slices of the dynptr are automatically
> + *                    invalidated after **bpf_dynptr_write**\ (). This is
> + *                    because writing may pull the skb and change the
> + *                    underlying packet buffer.
> + *
>   *                 *  For *flags*, please see the flags accepted by
>   *                    **bpf_skb_store_bytes**\ ().
>   *     Return
> --
> 2.30.2
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ