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:	Sun, 10 Jul 2016 15:56:02 -0500
From:	Tom Herbert <tom@...bertland.com>
To:	Brenden Blanco <bblanco@...mgrid.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Martin KaFai Lau <kafai@...com>,
	Jesper Dangaard Brouer <brouer@...hat.com>,
	Ari Saha <as754m@....com>,
	Alexei Starovoitov <alexei.starovoitov@...il.com>,
	Or Gerlitz <gerlitz.or@...il.com>,
	john fastabend <john.fastabend@...il.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Thomas Graf <tgraf@...g.ch>,
	Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH v6 01/12] bpf: add XDP prog type for early driver filter

On Thu, Jul 7, 2016 at 9:15 PM, Brenden Blanco <bblanco@...mgrid.com> wrote:
> Add a new bpf prog type that is intended to run in early stages of the
> packet rx path. Only minimal packet metadata will be available, hence a
> new context type, struct xdp_md, is exposed to userspace. So far only
> expose the packet start and end pointers, and only in read mode.
>
> An XDP program must return one of the well known enum values, all other
> return codes are reserved for future use. Unfortunately, this
> restriction is hard to enforce at verification time, so take the
> approach of warning at runtime when such programs are encountered. The
> driver can choose to implement unknown return codes however it wants,
> but must invoke the warning helper with the action value.
>
> Signed-off-by: Brenden Blanco <bblanco@...mgrid.com>
> ---
>  include/linux/filter.h   | 18 ++++++++++
>  include/uapi/linux/bpf.h | 19 ++++++++++
>  kernel/bpf/verifier.c    |  1 +
>  net/core/filter.c        | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 129 insertions(+)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 6fc31ef..522dbc9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -368,6 +368,11 @@ struct bpf_skb_data_end {
>         void *data_end;
>  };
>
> +struct xdp_buff {
> +       void *data;
> +       void *data_end;
> +};
> +
>  /* compute the linear packet data range [data, data_end) which
>   * will be accessed by cls_bpf and act_bpf programs
>   */
> @@ -429,6 +434,18 @@ static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
>         return BPF_PROG_RUN(prog, skb);
>  }
>
> +static inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
> +                                  struct xdp_buff *xdp)
> +{
> +       u32 ret;
> +
> +       rcu_read_lock();
> +       ret = BPF_PROG_RUN(prog, (void *)xdp);
> +       rcu_read_unlock();
> +
> +       return ret;
> +}
> +
>  static inline unsigned int bpf_prog_size(unsigned int proglen)
>  {
>         return max(sizeof(struct bpf_prog),
> @@ -509,6 +526,7 @@ bool bpf_helper_changes_skb_data(void *func);
>
>  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>                                        const struct bpf_insn *patch, u32 len);
> +void bpf_warn_invalid_xdp_action(int act);
>
>  #ifdef CONFIG_BPF_JIT
>  extern int bpf_jit_enable;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c14ca1c..5b47ac3 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -94,6 +94,7 @@ enum bpf_prog_type {
>         BPF_PROG_TYPE_SCHED_CLS,
>         BPF_PROG_TYPE_SCHED_ACT,
>         BPF_PROG_TYPE_TRACEPOINT,
> +       BPF_PROG_TYPE_XDP,
>  };
>
>  #define BPF_PSEUDO_MAP_FD      1
> @@ -430,4 +431,22 @@ struct bpf_tunnel_key {
>         __u32 tunnel_label;
>  };
>
> +/* User return codes for XDP prog type.
> + * A valid XDP program must return one of these defined values. All other
> + * return codes are reserved for future use. Unknown return codes will result
> + * in driver-dependent behavior.
> + */
> +enum xdp_action {
> +       XDP_DROP,
> +       XDP_PASS,
> +};
> +
> +/* user accessible metadata for XDP packet hook
> + * new fields must be added to the end of this structure
> + */
> +struct xdp_md {
> +       __u32 data;
> +       __u32 data_end;
> +};
> +
>  #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e206c21..a8d67d0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -713,6 +713,7 @@ static int check_ptr_alignment(struct verifier_env *env, struct reg_state *reg,
>         switch (env->prog->type) {
>         case BPF_PROG_TYPE_SCHED_CLS:
>         case BPF_PROG_TYPE_SCHED_ACT:
> +       case BPF_PROG_TYPE_XDP:
>                 break;
>         default:
>                 verbose("verifier is misconfigured\n");
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 10c4a2f..4ba446f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2369,6 +2369,12 @@ tc_cls_act_func_proto(enum bpf_func_id func_id)
>         }
>  }
>
> +static const struct bpf_func_proto *
> +xdp_func_proto(enum bpf_func_id func_id)
> +{
> +       return sk_filter_func_proto(func_id);
> +}
> +
>  static bool __is_valid_access(int off, int size, enum bpf_access_type type)
>  {
>         if (off < 0 || off >= sizeof(struct __sk_buff))

Can off be size_t to eliminate <0 check?

> @@ -2436,6 +2442,56 @@ static bool tc_cls_act_is_valid_access(int off, int size,
>         return __is_valid_access(off, size, type);
>  }
>
> +static bool __is_valid_xdp_access(int off, int size,
> +                                 enum bpf_access_type type)
> +{
> +       if (off < 0 || off >= sizeof(struct xdp_md))
> +               return false;
> +       if (off % size != 0)

off & 3 != 0

> +               return false;
> +       if (size != 4)
> +               return false;

If size must always be 4 why is it even an argument?

> +
> +       return true;
> +}
> +
> +static bool xdp_is_valid_access(int off, int size,
> +                               enum bpf_access_type type,
> +                               enum bpf_reg_type *reg_hint)
> +{
> +       if (type == BPF_WRITE)
> +               return false;
> +
> +       switch (off) {
> +       case offsetof(struct xdp_md, data):
> +               *reg_hint = PTR_TO_PACKET;
> +               break;
> +       case offsetof(struct xdp_md, data_end):
> +               *reg_hint = PTR_TO_PACKET_END;
> +               break;
case sizeof(int) for below?

> +       }
> +
> +       return __is_valid_xdp_access(off, size, type);
> +}
> +
> +void bpf_warn_invalid_xdp_action(int act)
> +{
> +       WARN_ONCE(1, "\n"
> +                    "*****************************************************\n"
> +                    "**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n"
> +                    "**                                               **\n"
> +                    "** XDP program returned unknown value %-10u **\n"
> +                    "**                                               **\n"
> +                    "** XDP programs must return a well-known return  **\n"
> +                    "** value. Invalid return values will result in   **\n"
> +                    "** undefined packet actions.                     **\n"
> +                    "**                                               **\n"
> +                    "**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n"
> +                    "*****************************************************\n",
> +                 act);

Seems a little verbose to be. Just do a simple WARN_ONCE and probably
more important to bump a counter. Also function should take the skb in
question as an argument in case we want to do more inspection or
logging in the future.

> +}
> +EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
> +
>  static u32 bpf_net_convert_ctx_access(enum bpf_access_type type, int dst_reg,
>                                       int src_reg, int ctx_off,
>                                       struct bpf_insn *insn_buf,
> @@ -2587,6 +2643,29 @@ static u32 bpf_net_convert_ctx_access(enum bpf_access_type type, int dst_reg,
>         return insn - insn_buf;
>  }
>
> +static u32 xdp_convert_ctx_access(enum bpf_access_type type, int dst_reg,
> +                                 int src_reg, int ctx_off,
> +                                 struct bpf_insn *insn_buf,
> +                                 struct bpf_prog *prog)
> +{
> +       struct bpf_insn *insn = insn_buf;
> +
> +       switch (ctx_off) {
> +       case offsetof(struct xdp_md, data):
> +               *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(FIELD_SIZEOF(struct xdp_buff, data)),
> +                                     dst_reg, src_reg,
> +                                     offsetof(struct xdp_buff, data));
> +               break;
> +       case offsetof(struct xdp_md, data_end):
> +               *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(FIELD_SIZEOF(struct xdp_buff, data_end)),
> +                                     dst_reg, src_reg,
> +                                     offsetof(struct xdp_buff, data_end));
> +               break;
> +       }
> +
> +       return insn - insn_buf;
> +}
> +
>  static const struct bpf_verifier_ops sk_filter_ops = {
>         .get_func_proto         = sk_filter_func_proto,
>         .is_valid_access        = sk_filter_is_valid_access,
> @@ -2599,6 +2678,12 @@ static const struct bpf_verifier_ops tc_cls_act_ops = {
>         .convert_ctx_access     = bpf_net_convert_ctx_access,
>  };
>
> +static const struct bpf_verifier_ops xdp_ops = {
> +       .get_func_proto         = xdp_func_proto,
> +       .is_valid_access        = xdp_is_valid_access,
> +       .convert_ctx_access     = xdp_convert_ctx_access,
> +};
> +
>  static struct bpf_prog_type_list sk_filter_type __read_mostly = {
>         .ops    = &sk_filter_ops,
>         .type   = BPF_PROG_TYPE_SOCKET_FILTER,
> @@ -2614,11 +2699,17 @@ static struct bpf_prog_type_list sched_act_type __read_mostly = {
>         .type   = BPF_PROG_TYPE_SCHED_ACT,
>  };
>
> +static struct bpf_prog_type_list xdp_type __read_mostly = {
> +       .ops    = &xdp_ops,
> +       .type   = BPF_PROG_TYPE_XDP,
> +};
> +
>  static int __init register_sk_filter_ops(void)
>  {
>         bpf_register_prog_type(&sk_filter_type);
>         bpf_register_prog_type(&sched_cls_type);
>         bpf_register_prog_type(&sched_act_type);
> +       bpf_register_prog_type(&xdp_type);
>
>         return 0;
>  }
> --
> 2.8.2
>

Powered by blists - more mailing lists