diff mbox series

[ovs-dev,v2] flow: Count and dump invalid IP packets.

Message ID 20210416120631.4584-1-david.marchand@redhat.com
State Accepted
Headers show
Series [ovs-dev,v2] flow: Count and dump invalid IP packets. | expand

Commit Message

David Marchand April 16, 2021, 12:06 p.m. UTC 
Skipping further processing of invalid IP packets helps avoid crashes
but it does not help to figure out if the malformed packets are still
present on the network.

Add coverage counters for IPv4 and IPv6 sanity checks so that we know
there are some invalid packets.

Dump such whole packets in debug mode.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
---
Changes since v1:
- changed counter names to reflect they are part of miniflow_extract,

---
 lib/flow.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Flavio Leitner April 16, 2021, 12:44 p.m. UTC | #1 
On Fri, Apr 16, 2021 at 02:06:31PM +0200, David Marchand wrote:
> Skipping further processing of invalid IP packets helps avoid crashes
> but it does not help to figure out if the malformed packets are still
> present on the network.
> 
> Add coverage counters for IPv4 and IPv6 sanity checks so that we know
> there are some invalid packets.
> 
> Dump such whole packets in debug mode.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> ---

The patch looks good to me.

Generated log dumping the packet correctly:
2021-04-16T12:37:25.525Z|00004|flow(handler21)|DBG|invalid packet for ipv6_sanity_check: port 1, size 86
00000000  33 33 ff 00 00 02 7a d0-49 c1 c0 e9 86 dd 60 00                                                                                    
00000010  00 00 00 21 3a ff fe 80-00 00 00 00 00 00 78 d0                                                                                    
00000020  49 ff fe c1 c0 e9 ff 02-00 00 00 00 00 00 00 00                                                                                    
00000030  00 01 ff 00 00 02 87 00-74 a2 00 00 00 00 fe 80                                                                                    
00000040  00 00 00 00 00 00 00 00-00 00 00 00 00 02 01 01                                                                                    
00000050  7a d0 49 c1 c0 e9 

# ovs-appctl coverage/show | grep miniflow
miniflow_extract_ipv6_pkt_len_error   0.0/sec     0.000/sec 0.0011/sec   total: 4

Acked-by: Flavio Leitner <fbl@sysclose.org>
Ilya Maximets July 16, 2021, 1:24 p.m. UTC | #2 
On 4/16/21 2:44 PM, Flavio Leitner wrote:
> On Fri, Apr 16, 2021 at 02:06:31PM +0200, David Marchand wrote:
>> Skipping further processing of invalid IP packets helps avoid crashes
>> but it does not help to figure out if the malformed packets are still
>> present on the network.
>>
>> Add coverage counters for IPv4 and IPv6 sanity checks so that we know
>> there are some invalid packets.
>>
>> Dump such whole packets in debug mode.
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> Acked-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
> 
> The patch looks good to me.
> 
> Generated log dumping the packet correctly:
> 2021-04-16T12:37:25.525Z|00004|flow(handler21)|DBG|invalid packet for ipv6_sanity_check: port 1, size 86
> 00000000  33 33 ff 00 00 02 7a d0-49 c1 c0 e9 86 dd 60 00                                                                                    
> 00000010  00 00 00 21 3a ff fe 80-00 00 00 00 00 00 78 d0                                                                                    
> 00000020  49 ff fe c1 c0 e9 ff 02-00 00 00 00 00 00 00 00                                                                                    
> 00000030  00 01 ff 00 00 02 87 00-74 a2 00 00 00 00 fe 80                                                                                    
> 00000040  00 00 00 00 00 00 00 00-00 00 00 00 00 02 01 01                                                                                    
> 00000050  7a d0 49 c1 c0 e9 
> 
> # ovs-appctl coverage/show | grep miniflow
> miniflow_extract_ipv6_pkt_len_error   0.0/sec     0.000/sec 0.0011/sec   total: 4
> 
> Acked-by: Flavio Leitner <fbl@sysclose.org>


Thanks, David, Eelco and Flavio!
This is an important change taking into account recent security issues
due to malformed packets.  Applied to master.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/flow.c b/lib/flow.c
index 729d59b1b3..89837de95d 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -44,8 +44,15 @@ 
 #include "openvswitch/nsh.h"
 #include "ovs-router.h"
 #include "lib/netdev-provider.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(flow);
 
 COVERAGE_DEFINE(flow_extract);
+COVERAGE_DEFINE(miniflow_extract_ipv4_pkt_len_error);
+COVERAGE_DEFINE(miniflow_extract_ipv4_pkt_too_short);
+COVERAGE_DEFINE(miniflow_extract_ipv6_pkt_len_error);
+COVERAGE_DEFINE(miniflow_extract_ipv6_pkt_too_short);
 COVERAGE_DEFINE(miniflow_malloc);
 
 /* U64 indices for segmented flow classification. */
@@ -645,17 +652,20 @@  ipv4_sanity_check(const struct ip_header *nh, size_t size,
     uint16_t tot_len;
 
     if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
+        COVERAGE_INC(miniflow_extract_ipv4_pkt_too_short);
         return false;
     }
     ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
 
     if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN || size < ip_len)) {
+        COVERAGE_INC(miniflow_extract_ipv4_pkt_len_error);
         return false;
     }
 
     tot_len = ntohs(nh->ip_tot_len);
     if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len ||
                 size - tot_len > UINT16_MAX)) {
+        COVERAGE_INC(miniflow_extract_ipv4_pkt_len_error);
         return false;
     }
 
@@ -686,21 +696,41 @@  ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
     uint16_t plen;
 
     if (OVS_UNLIKELY(size < sizeof *nh)) {
+        COVERAGE_INC(miniflow_extract_ipv6_pkt_too_short);
         return false;
     }
 
     plen = ntohs(nh->ip6_plen);
     if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
+        COVERAGE_INC(miniflow_extract_ipv6_pkt_len_error);
         return false;
     }
 
     if (OVS_UNLIKELY(size - (plen + IPV6_HEADER_LEN) > UINT16_MAX)) {
+        COVERAGE_INC(miniflow_extract_ipv6_pkt_len_error);
         return false;
     }
 
     return true;
 }
 
+static void
+dump_invalid_packet(struct dp_packet *packet, const char *reason)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    struct ds ds = DS_EMPTY_INITIALIZER;
+    size_t size;
+
+    if (VLOG_DROP_DBG(&rl)) {
+        return;
+    }
+    size = dp_packet_size(packet);
+    ds_put_hex_dump(&ds, dp_packet_data(packet), size, 0, false);
+    VLOG_DBG("invalid packet for %s: port %"PRIu32", size %"PRIuSIZE"\n%s",
+             reason, packet->md.in_port.odp_port, size, ds_cstr(&ds));
+    ds_destroy(&ds);
+}
+
 /* Initializes 'dst' from 'packet' and 'md', taking the packet type into
  * account.  'dst' must have enough space for FLOW_U64S * 8 bytes.
  *
@@ -850,6 +880,9 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
         uint16_t tot_len;
 
         if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len, &tot_len))) {
+            if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) {
+                dump_invalid_packet(packet, "ipv4_sanity_check");
+            }
             goto out;
         }
         dp_packet_set_l2_pad_size(packet, size - tot_len);
@@ -880,6 +913,9 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
         uint16_t plen;
 
         if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
+            if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) {
+                dump_invalid_packet(packet, "ipv6_sanity_check");
+            }
             goto out;
         }
         data_pull(&data, &size, sizeof *nh);
@@ -1114,6 +1150,9 @@  parse_tcp_flags(struct dp_packet *packet)
         uint16_t tot_len;
 
         if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len, &tot_len))) {
+            if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) {
+                dump_invalid_packet(packet, "ipv4_sanity_check");
+            }
             return 0;
         }
         dp_packet_set_l2_pad_size(packet, size - tot_len);
@@ -1127,6 +1166,9 @@  parse_tcp_flags(struct dp_packet *packet)
         uint16_t plen;
 
         if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
+            if (OVS_UNLIKELY(VLOG_IS_DBG_ENABLED())) {
+                dump_invalid_packet(packet, "ipv6_sanity_check");
+            }
             return 0;
         }
         data_pull(&data, &size, sizeof *nh);