netdev - Re: [PATCH net-next v2 3/4] net: check tunnel option type in tunnel flags

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: <20180628095452.6f23fdf4@cakuba.netronome.com>
Date:   Thu, 28 Jun 2018 09:54:52 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>, davem@...emloft.net,
        Roopa Prabhu <roopa@...ulusnetworks.com>, jiri@...nulli.us,
        jhs@...atatu.com, xiyou.wangcong@...il.com,
        oss-drivers@...ronome.com, netdev@...r.kernel.org,
        Pieter Jansen van Vuuren 
        <pieter.jansenvanvuuren@...ronome.com>
Subject: Re: [PATCH net-next v2 3/4] net: check tunnel option type in tunnel
 flags

On Thu, 28 Jun 2018 09:42:06 +0200, Jiri Benc wrote:
> On Wed, 27 Jun 2018 11:49:49 +0200, Daniel Borkmann wrote:
> > Looks good to me, and yes in BPF case a mask like TUNNEL_OPTIONS_PRESENT is
> > right approach since this is opaque info and solely defined by the BPF prog
> > that is using the generic helper.  
> 
> Wouldn't it make sense to introduce some safeguards here (in a backward
> compatible way, of course)? It's easy to mistakenly set data for a
> different tunnel type in a BPF program and then be surprised by the
> result. It might help users if such usage was detected by the kernel,
> one way or another.

Well, that's how it works today ;)

> I'm thinking about something like the BPF program voluntarily
> specifying the type of the data; if not specified, the wildcard would be
> used as it is now.

Hmm... in practice we could steal top bits of the size parameter for
some flags, since it seems to be limited to values < 256 today?  Is it
worth it?

It would look something along the lines of:

---

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 59b19b6a40d7..194b40efa8e8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2213,6 +2213,13 @@ enum bpf_func_id {
 /* BPF_FUNC_perf_event_output for sk_buff input context. */
 #define BPF_F_CTXLEN_MASK              (0xfffffULL << 32)
 
+#define BPF_F_TUN_VXLAN                        (1U << 31)
+#define BPF_F_TUN_GENEVE               (1U << 30)
+#define BPF_F_TUN_ERSPAN               (1U << 29)
+#define BPF_F_TUN_FLAGS_ALL            (BPF_F_TUN_VXLAN | \
+                                        BPF_F_TUN_GENEVE | \
+                                        BPF_F_TUN_ERSPAN)
+
 /* Mode for BPF_FUNC_skb_adjust_room helper. */
 enum bpf_adj_room_mode {
        BPF_ADJ_ROOM_NET,
diff --git a/net/core/filter.c b/net/core/filter.c
index dade922678f6..cc592a1e8945 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3576,6 +3576,22 @@ BPF_CALL_3(bpf_skb_set_tunnel_opt, struct sk_buff *, skb,
 {
        struct ip_tunnel_info *info = skb_tunnel_info(skb);
        const struct metadata_dst *md = this_cpu_ptr(md_dst);
+       __be16 tun_flags;
+       u32 flags;
+
+       BUILD_BUG_ON(BPF_F_TUN_FLAGS_ALL & IP_TUNNEL_OPTS_MAX);
+
+       flags = size & BPF_F_TUN_FLAGS_ALL;
+       size &= ~flags;
+       if (flags & BPF_F_TUN_VXLAN)
+               tun_flags |= TUNNEL_VXLAN_OPT;
+       if (flags & BPF_F_TUN_GENEVE)
+               tun_flags |= TUNNEL_GENEVE_OPT;
+       if (flags & BPF_F_TUN_ERSPAN)
+               tun_flags |= TUNNEL_ERSPAN_OPT;
+       /* User didn't specify the tunnel type, for backward compat set all */
+       if (!(tun_flags & TUNNEL_OPTIONS_PRESENT))
+               tun_flags |= TUNNEL_OPTIONS_PRESENT;
 
        if (unlikely(info != &md->u.tun_info || (size & (sizeof(u32) - 1))))
                return -EINVAL;