nat: handle ED in2out ICMP errors with workers

Type: fix

With endpoint dependent NAT44, When there are multiple workers we look
for a flow which matches the packet in order to figure out which worker
should handle the packet. If the packet is an ICMP error, it may be
associated with an existing flow by inspecting the L3/L4 headers that
are included in the message payload.

This was not being done for in2out packets in
nat44_ed_get_in2out_worker_index(), so some packets which were related
to an open session were not being associated with that session and were
being passed to a different thread than the one where the session was
created. Later on, when the packet was processed by the fast path in2out
node, the L3/L4 headers in the payload are inspected and the fast path
node finds the existing session. Since that session is owned by a
different thread than the one the packet is being processed by, the
in2out fast path node can potentially access the wrong session and/or
memory adjacent to the session pool. This can cause a SEGV.

Make nat44_ed_get_in2out_worker_index() look at the inner headers when
processing an ICMP error. THis is already done in
nat44_ed_get_out2in_worker_index() and in the fast path in2out node.

Change-Id: Icdc1abebcbce452ee7be7cb23fc563e09bf575f2
Signed-off-by: Matthew Smith <mgsmith@netgate.com>

2 files changed, 67 insertions, 0 deletions

diff --git a/src/plugins/nat/nat44-ed/nat44_ed.c b/src/plugins/nat/nat44-ed/nat44_ed.c
index ad78dc2dd1f..b9dbe6ebeca 100644
--- a/src/plugins/nat/nat44-ed/nat44_ed.c
+++ b/src/plugins/nat/nat44-ed/nat44_ed.c
@@ -2854,6 +2854,28 @@ nat44_ed_get_in2out_worker_index (vlib_buffer_t *b, ip4_header_t *ip,
 	    }
 	}
 
+      if (PREDICT_FALSE (ip->protocol == IP_PROTOCOL_ICMP))
+	{
+	  ip4_address_t lookup_saddr, lookup_daddr;
+	  u16 lookup_sport, lookup_dport;
+	  u8 lookup_protocol;
+
+	  if (!nat_get_icmp_session_lookup_values (
+		b, ip, &lookup_saddr, &lookup_sport, &lookup_daddr,
+		&lookup_dport, &lookup_protocol))
+	    {
+	      init_ed_k (&kv16, lookup_saddr, lookup_sport, lookup_daddr,
+			 lookup_dport, rx_fib_index, lookup_protocol);
+	      if (!clib_bihash_search_16_8 (&sm->flow_hash, &kv16, &value16))
+		{
+		  next_worker_index = ed_value_get_thread_index (&value16);
+		  vnet_buffer2 (b)->nat.cached_session_index =
+		    ed_value_get_session_index (&value16);
+		  goto out;
+		}
+	    }
+	}
+
       init_ed_k (&kv16, ip->src_address, vnet_buffer (b)->ip.reass.l4_src_port,
 		 ip->dst_address, vnet_buffer (b)->ip.reass.l4_dst_port,
 		 fib_index, ip->protocol);
diff --git a/test/test_nat44_ed.py b/test/test_nat44_ed.py
index 258bee3a1f6..ec8d7c8d159 100644
--- a/test/test_nat44_ed.py
+++ b/test/test_nat44_ed.py
@@ -3852,6 +3852,51 @@ class TestNAT44EDMW(TestNAT44ED):
             self.logger.error(ppp("Unexpected or invalid packet:", p))
             raise
 
+    def test_icmp_error_fwd_outbound(self):
+        """ NAT44ED ICMP error outbound with forwarding enabled """
+
+        # Ensure that an outbound ICMP error message is properly associated
+        # with the inbound forward bypass session it is related to.
+        payload = "H" * 10
+
+        self.nat_add_address(self.nat_addr)
+        self.nat_add_inside_interface(self.pg0)
+        self.nat_add_outside_interface(self.pg1)
+
+        # enable forwarding and initiate connection out2in
+        self.vapi.nat44_forwarding_enable_disable(enable=1)
+        p1 = (Ether(src=self.pg1.remote_mac, dst=self.pg1.local_mac) /
+              IP(src=self.pg1.remote_ip4, dst=self.pg0.remote_ip4) /
+              UDP(sport=21, dport=20) / payload)
+
+        self.pg1.add_stream(p1)
+        self.pg_enable_capture(self.pg_interfaces)
+        self.pg_start()
+        capture = self.pg0.get_capture(1)[0]
+
+        self.logger.info(self.vapi.cli("show nat44 sessions"))
+
+        # reply with ICMP error message in2out
+        # We cannot reliably retrieve forward bypass sessions via the API.
+        # session dumps for a user will only look on the worker that the
+        # user is supposed to be mapped to in2out. The forward bypass session
+        # is not necessarily created on that worker.
+        p2 = (Ether(src=self.pg0.remote_mac, dst=self.pg0.local_mac) /
+              IP(src=self.pg0.remote_ip4, dst=self.pg1.remote_ip4) /
+              ICMP(type='dest-unreach', code='port-unreachable') /
+              capture[IP:])
+
+        self.pg0.add_stream(p2)
+        self.pg_enable_capture(self.pg_interfaces)
+        self.pg_start()
+        capture = self.pg1.get_capture(1)[0]
+
+        self.logger.info(self.vapi.cli("show nat44 sessions"))
+
+        self.logger.info(ppp("p1 packet:", p1))
+        self.logger.info(ppp("p2 packet:", p2))
+        self.logger.info(ppp("capture packet:", capture))
+
 
 if __name__ == '__main__':
     unittest.main(testRunner=VppTestRunner)