lb: crash or infinite-tail-recursion for ip-ip packets with ihl=0

[awelzel]

Hello @giuseppelettieri and others,

we've observed lb spinning at 100% CPU (or crashing - depending on the optimization level) when hashing IP-IP packets with a IP header length set to 0.

Find attached a single packet pcap that reproduces the issue (tested with FreeBSD 13 and Linux VMs).

The following netsniff-ng trafgen command creates a pcap that can be used for reproduction (seems I can't attach a pcap..):

$ trafgen -n 1 '{ipv4(saddr=1.2.3.4, daddr=5.6.7.8, proto=4 ihl=0), ipv4(saddr=drnd(), daddr=drnd())}' -o ip-ip-ihl0.pcap

Steps for reproducing, start lb and replay single packet.

$ lb -i in{0/x -p out:1
438.751629 main [590] interface is in{0/x
438.766940 main [697] successfully opened netmap:in{0/x
438.767028 main [768] opening pipe named netmap:out{0/xT@2
438.767956 main [783] successfully opened pipe #1 netmap:out{0/xT@2 (tx slots: 1024)
438.768034 main [788] zerocopy enabled

Other terminal:

$ tcpreplay -i 'netmap:in}0' ip-ip-ihl0.pcap 
...
Statistics for network device: 
        Successful packets:        1

When compiled without optimizations, lb crashes due to a stack overflow. When compile with -O2 as the default, it is spinning at 100% due to endless-tail-recursion in decode_ip_n_hash.

We've applied the following minimum patch. It produces 0 for such packets has hash, but also counts them as non-ip which is a bit unfortuante.

diff --git a/apps/lb/pkt_hash.c b/apps/lb/pkt_hash.c
index 3071935e..52f7c9ba 100644
--- a/apps/lb/pkt_hash.c
+++ b/apps/lb/pkt_hash.c
@@ -176,8 +176,9 @@ decode_ip_n_hash(const struct ip *iph, uint8_t hash_split, uint8_t seed)
                        break;
                case IPPROTO_IPIP:
                        /* tunneling */
-                       rc = decode_ip_n_hash((const struct ip *)((const uint8_t *)iph + (iph->ip_hl<<2)),
-                                             hash_split, seed);
+                       if (iph->ip_hl)
+                               rc = decode_ip_n_hash((const struct ip *)((const uint8_t *)iph + (iph->ip_hl<<2)),
+                                                     hash_split, seed);

Happy to open an MR, but wondering if you have other/better fixes in mind.

[giuseppelettieri]

Well, we may argue that it is indeed not ip, since it is malformed. Maybe we may just add a if (iph->ip_hl < 5 || iph->ip_hl * 4 > iph->ip_len) return 0; at the beginning of the function. Objections?

[awelzel]

@giuseppelettieri , I've created an MR with you suggestion.

It might be nice to also add validation to stay within the bounds of the given packet being looked at, but that would be a fairly intrusive change.