[ovs-dev] [patch v3 2/4] conntrack: Fix fragmentation checks.

Darrell Ball dlu998 at gmail.com
Thu Jun 28 22:39:29 UTC 2018


The ipv4 fragmentation check is broken and allows fragments through.
There were fragile and poorly maintainable checks in extract_l3_ipv*
designed to save a few cycles.  The checks make assumptions about what
sanity checks may have been done and could be skipped based on inferring
from the value of another paramater that should be unrelated (l4
pointer needing assignment).  Since the benefit is minimal, remove
the special checks and always do sanity checks.  Some basic
optimization is added for the sanity checks and some parameter
ordering is changed for the functions being touched.

Four tests are added to better maintain fragmentation support.

This needs backporting to 2.9.

Fixes: c8b1ad49da68("conntrack: Reorder sanity checks in extract_l3_ipvx().")
Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
Signed-off-by: Darrell Ball <dlu998 at gmail.com>
---
 lib/conntrack.c         | 95 +++++++++++++++++++++----------------------------
 tests/system-traffic.at | 92 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 55 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 82229fa..825f1ad 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -123,13 +123,14 @@ static uint8_t
 reverse_icmp_type(uint8_t type);
 static uint8_t
 reverse_icmp6_type(uint8_t type);
-static inline bool
-extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
-                const char **new_data, bool validate_checksum);
-static inline bool
-extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,
-                const char **new_data);
-
+static inline bool extract_l3_ipv4(const void *data, size_t size,
+                                   bool validate_checksum,
+                                   struct conn_key *key,
+                                   const char **new_data);
+
+static inline bool extract_l3_ipv6(const void *data, size_t size,
+                                   struct conn_key *key,
+                                   const char **new_data);
 static struct alg_exp_node *
 expectation_lookup(struct hmap *alg_expectations, const struct conn_key *key,
                    uint32_t basis, bool src_ip_wc);
@@ -648,8 +649,8 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
         struct ip_header *nh = dp_packet_l3(pkt);
         struct icmp_header *icmp = dp_packet_l4(pkt);
         struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
-        extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - pad,
-                        &inner_l4, false);
+        extract_l3_ipv4(inner_l3, tail - (char *)inner_l3 - pad, false,
+                        &inner_key, &inner_l4);
         pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
         pkt->l4_ofs += inner_l4 - (char *) icmp;
 
@@ -669,9 +670,8 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
         struct icmp6_error_header *icmp6 = dp_packet_l4(pkt);
         struct ovs_16aligned_ip6_hdr *inner_l3_6 =
             (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1);
-        extract_l3_ipv6(&inner_key, inner_l3_6,
-                        tail - ((char *)inner_l3_6) - pad,
-                        &inner_l4);
+        extract_l3_ipv6(inner_l3_6, tail - ((char *)inner_l3_6) - pad,
+                        &inner_key, &inner_l4);
         pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6;
         pkt->l4_ofs += inner_l4 - (char *) icmp6;
 
@@ -1497,46 +1497,37 @@ clean_thread_main(void *f_)
     return NULL;
 }
 
-/* Key extraction */
-
-/* The function stores a pointer to the first byte after the header in
- * '*new_data', if 'new_data' is not NULL.  If it is NULL, the caller is
- * not interested in the header's tail,  meaning that the header has
- * already been parsed (e.g. by flow_extract): we take this as a hint to
- * save a few checks.  If 'validate_checksum' is true, the function returns
- * false if the IPv4 checksum is invalid. */
 static inline bool
-extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
-                const char **new_data, bool validate_checksum)
+extract_l3_ipv4(const void *data, size_t size, bool validate_checksum,
+                struct conn_key *key, const char **new_data)
 {
-    if (new_data) {
-        if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
-            return false;
-        }
+    if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
+        return false;
     }
 
     const struct ip_header *ip = data;
     size_t ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
 
-    if (new_data) {
-        if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
-            return false;
-        }
-        if (OVS_UNLIKELY(size < ip_len)) {
-            return false;
-        }
+    if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
+        return false;
+    }
 
-        if (IP_IS_FRAGMENT(ip->ip_frag_off)) {
-            return false;
-        }
+    if (OVS_UNLIKELY(size < ip_len)) {
+        return false;
+    }
 
-        *new_data = (char *) data + ip_len;
+    if (OVS_UNLIKELY(validate_checksum && csum(data, ip_len) != 0)) {
+        return false;
     }
 
-    if (validate_checksum && csum(data, ip_len) != 0) {
+    if (OVS_UNLIKELY(IP_IS_FRAGMENT(ip->ip_frag_off))) {
         return false;
     }
 
+    if (new_data) {
+        *new_data = (char *) data + ip_len;
+    }
+
     key->src.addr.ipv4 = ip->ip_src;
     key->dst.addr.ipv4 = ip->ip_dst;
     key->nw_proto = ip->ip_proto;
@@ -1544,21 +1535,14 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
     return true;
 }
 
-/* The function stores a pointer to the first byte after the header in
- * '*new_data', if 'new_data' is not NULL.  If it is NULL, the caller is
- * not interested in the header's tail,  meaning that the header has
- * already been parsed (e.g. by flow_extract): we take this as a hint to
- * save a few checks. */
 static inline bool
-extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,
+extract_l3_ipv6(const void *data, size_t size, struct conn_key *key,
                 const char **new_data)
 {
     const struct ovs_16aligned_ip6_hdr *ip6 = data;
 
-    if (new_data) {
-        if (OVS_UNLIKELY(size < sizeof *ip6)) {
-            return false;
-        }
+    if (OVS_UNLIKELY(size < sizeof *ip6)) {
+        return false;
     }
 
     data = ip6 + 1;
@@ -1566,11 +1550,12 @@ extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,
     uint8_t nw_proto = ip6->ip6_nxt;
     uint8_t nw_frag = 0;
 
-    if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag)) {
+    if (OVS_UNLIKELY(!parse_ipv6_ext_hdrs(&data, &size, &nw_proto,
+                                          &nw_frag))) {
         return false;
     }
 
-    if (nw_frag) {
+    if (OVS_UNLIKELY(nw_frag)) {
         return false;
     }
 
@@ -1758,7 +1743,7 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
 
         memset(&inner_key, 0, sizeof inner_key);
         inner_key.dl_type = htons(ETH_TYPE_IP);
-        bool ok = extract_l3_ipv4(&inner_key, l3, tail - l3, &l4, false);
+        bool ok = extract_l3_ipv4(l3, tail - l3, false, &inner_key, &l4);
         if (!ok) {
             return false;
         }
@@ -1843,7 +1828,7 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
 
         memset(&inner_key, 0, sizeof inner_key);
         inner_key.dl_type = htons(ETH_TYPE_IPV6);
-        bool ok = extract_l3_ipv6(&inner_key, l3, tail - l3, &l4);
+        bool ok = extract_l3_ipv6(l3, tail - l3, &inner_key, &l4);
         if (!ok) {
             return false;
         }
@@ -1964,11 +1949,11 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         } else {
             bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt);
             /* Validate the checksum only when hwol is not supported. */
-            ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
-                                 !hwol_good_l3_csum);
+            ok = extract_l3_ipv4(l3, tail - (char *) l3, !hwol_good_l3_csum,
+                                 &ctx->key,  NULL);
         }
     } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
-        ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3, NULL);
+        ok = extract_l3_ipv6(l3, tail - (char *) l3, &ctx->key, NULL);
     } else {
         ok = false;
     }
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 7080efe..29ff91f 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2057,6 +2057,52 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.255.2.2 | FORMAT_PI
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - IPv4 fragmentation incomplete reassembled packet])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+AT_DATA([bundle.txt], [dnl
+packet-out in_port=1, packet=50540000000a5054000000090800450001a400012000001183440a0101010a01010200010002000800000304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
+])
+
+AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+dnl Uses same first fragment as above 'incomplete reassembled packet' test.
+AT_SETUP([conntrack - IPv4 fragmentation with fragments specified])
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_FRAG()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+AT_DATA([bundle.txt], [dnl
+packet-out in_port=1, packet=50540000000a5054000000090800450001a400012000001183440a0101010a01010200010002000800000304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
+packet-out in_port=1, packet=50540000000a505400000009080045000030000100320011a4860a0101010a01010200010002000800000010203040506070809000010203040506070809, actions=ct(commit)
+])
+
+AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - IPv6 fragmentation])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_FRAG()
@@ -2235,6 +2281,52 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00:ffff::4 | FORMAT
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - IPv6 fragmentation incomplete reassembled packet])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "fc00::1/96")
+ADD_VETH(p1, at_ns1, br0, "fc00::2/96")
+
+AT_DATA([bundle.txt], [dnl
+packet-out in_port=1, packet=50540000000a50540000000986dd6000000005102cfffc000000000000000000000000000001fc0000000000000000000000000000021100000100000001000100020008f62900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010\2030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080
 900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607\0809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020
 304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020\304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
+])
+
+AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+dnl Uses same first fragment as above 'incomplete reassembled packet' test.
+AT_SETUP([conntrack - IPv6 fragmentation with fragments specified])
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_FRAG()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "fc00::1/96")
+ADD_VETH(p1, at_ns1, br0, "fc00::2/96")
+
+AT_DATA([bundle.txt], [dnl
+packet-out in_port=1, packet=50540000000a50540000000986dd6000000005102cfffc000000000000000000000000000001fc0000000000000000000000000000021100000100000001000100020008cdf30001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000\10203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080
 9000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405\060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020
 304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090\001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809000102030405060708090001020304050607080900010203040506070809,  actions=ct(commit)
+packet-out in_port=1, packet=50540000000a50540000000986dd6000000000242cfffc000000000000000000000000000001fc000000000000000000000000000002110005080000000100010002000800000001020304050607080900010203040506070809, actions=ct(commit)
+])
+
+AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl
+udp,orig=(src=fc00::1,dst=fc00::2,sport=<cleared>,dport=<cleared>),reply=(src=fc00::2,dst=fc00::1,sport=<cleared>,dport=<cleared>)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - Fragmentation over vxlan])
 OVS_CHECK_VXLAN()
 CHECK_CONNTRACK()
-- 
1.9.1



More information about the dev mailing list