From: bluhm <bluhm@openbsd.org>
Date: Wed, 24 Nov 2021 18:48:33 +0000 (+0000)
Subject: When sending ICMP packets for IPsec path MTU discovery, the first
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=6d36119535d77e3442deb6d8127913d1fe2e309d;p=openbsd

When sending ICMP packets for IPsec path MTU discovery, the first
ICMP packet could be wrong.  The mtu was taken from the loopback
interface as the tdb mtu was copied to the route too late.  Without
crypto task, ipsp_process_packet() returns the EMSGSIZE error
earlier.  Immediately update tdb and route mtu.
IPv4 part from markus@; OK tobhe@
---

diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index a739f1fd356..0d92e347fae 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_output.c,v 1.374 2021/07/27 17:13:03 mvs Exp $	*/
+/*	$OpenBSD: ip_output.c,v 1.375 2021/11/24 18:48:33 bluhm Exp $	*/
 /*	$NetBSD: ip_output.c,v 1.28 1996/02/13 23:43:07 christos Exp $	*/
 
 /*
@@ -86,13 +86,11 @@ static __inline u_int16_t __attribute__((__unused__))
 void in_delayed_cksum(struct mbuf *);
 int in_ifcap_cksum(struct mbuf *, struct ifnet *, int);
 
-#ifdef IPSEC
-struct tdb *
-ip_output_ipsec_lookup(struct mbuf *m, int hlen, int *error, struct inpcb *inp,
-    int ipsecflowinfo);
-int
-ip_output_ipsec_send(struct tdb *, struct mbuf *, struct route *, int);
-#endif /* IPSEC */
+struct tdb *ip_output_ipsec_lookup(struct mbuf *m, int hlen, int *error,
+    struct inpcb *inp, int ipsecflowinfo);
+void ip_output_ipsec_pmtu_update(struct tdb *, struct route *, struct in_addr,
+    int, int);
+int ip_output_ipsec_send(struct tdb *, struct mbuf *, struct route *, int);
 
 /*
  * IP output.  The packet in mbuf chain m contains a skeletal IP
@@ -563,6 +561,36 @@ ip_output_ipsec_lookup(struct mbuf *m, int hlen, int *error, struct inpcb *inp,
 	return tdb;
 }
 
+void
+ip_output_ipsec_pmtu_update(struct tdb *tdb, struct route *ro,
+    struct in_addr dst, int rtableid, int transportmode)
+{
+	struct rtentry *rt = NULL;
+	int rt_mtucloned = 0;
+
+	/* Find a host route to store the mtu in */
+	if (ro != NULL)
+		rt = ro->ro_rt;
+	/* but don't add a PMTU route for transport mode SAs */
+	if (transportmode)
+		rt = NULL;
+	else if (rt == NULL || (rt->rt_flags & RTF_HOST) == 0) {
+		rt = icmp_mtudisc_clone(dst, rtableid, 1);
+		rt_mtucloned = 1;
+	}
+	DPRINTF("spi %08x mtu %d rt %p cloned %d",
+	    ntohl(tdb->tdb_spi), tdb->tdb_mtu, rt, rt_mtucloned);
+	if (rt != NULL) {
+		rt->rt_mtu = tdb->tdb_mtu;
+		if (ro != NULL && ro->ro_rt != NULL) {
+			rtfree(ro->ro_rt);
+			ro->ro_rt = rtalloc(&ro->ro_dst, RT_RESOLVE, rtableid);
+		}
+		if (rt_mtucloned)
+			rtfree(rt);
+	}
+}
+
 int
 ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route *ro, int fwd)
 {
@@ -570,7 +598,8 @@ ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route *ro, int fwd)
 	struct ifnet *encif;
 #endif
 	struct ip *ip;
-	int error;
+	struct in_addr dst;
+	int error, rtableid;
 
 #if NPF > 0
 	/*
@@ -595,39 +624,17 @@ ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route *ro, int fwd)
 
 	/* Check if we are allowed to fragment */
 	ip = mtod(m, struct ip *);
+	dst = ip->ip_dst;
+	rtableid = m->m_pkthdr.ph_rtableid;
 	if (ip_mtudisc && (ip->ip_off & htons(IP_DF)) && tdb->tdb_mtu &&
 	    ntohs(ip->ip_len) > tdb->tdb_mtu &&
 	    tdb->tdb_mtutimeout > gettime()) {
-		struct rtentry *rt = NULL;
-		int rt_mtucloned = 0;
-		int transportmode = 0;
+		int transportmode;
 
 		transportmode = (tdb->tdb_dst.sa.sa_family == AF_INET) &&
-		    (tdb->tdb_dst.sin.sin_addr.s_addr == ip->ip_dst.s_addr);
-
-		/* Find a host route to store the mtu in */
-		if (ro != NULL)
-			rt = ro->ro_rt;
-		/* but don't add a PMTU route for transport mode SAs */
-		if (transportmode)
-			rt = NULL;
-		else if (rt == NULL || (rt->rt_flags & RTF_HOST) == 0) {
-			rt = icmp_mtudisc_clone(ip->ip_dst,
-			    m->m_pkthdr.ph_rtableid, 1);
-			rt_mtucloned = 1;
-		}
-		DPRINTF("spi %08x mtu %d rt %p cloned %d",
-		    ntohl(tdb->tdb_spi), tdb->tdb_mtu, rt, rt_mtucloned);
-		if (rt != NULL) {
-			rt->rt_mtu = tdb->tdb_mtu;
-			if (ro != NULL && ro->ro_rt != NULL) {
-				rtfree(ro->ro_rt);
-				ro->ro_rt = rtalloc(&ro->ro_dst, RT_RESOLVE,
-				    m->m_pkthdr.ph_rtableid);
-			}
-			if (rt_mtucloned)
-				rtfree(rt);
-		}
+		    (tdb->tdb_dst.sin.sin_addr.s_addr == dst.s_addr);
+		ip_output_ipsec_pmtu_update(tdb, ro, dst, rtableid,
+		    transportmode);
 		ipsec_adjust_mtu(m, tdb->tdb_mtu);
 		m_freem(m);
 		return EMSGSIZE;
@@ -648,6 +655,8 @@ ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route *ro, int fwd)
 		ipsecstat_inc(ipsec_odrops);
 		tdb->tdb_odrops++;
 	}
+	if (ip_mtudisc && error == EMSGSIZE)
+		ip_output_ipsec_pmtu_update(tdb, ro, dst, rtableid, 0);
 	return error;
 }
 #endif /* IPSEC */
diff --git a/sys/netinet6/ip6_output.c b/sys/netinet6/ip6_output.c
index fe75a0d08fa..974147b0e57 100644
--- a/sys/netinet6/ip6_output.c
+++ b/sys/netinet6/ip6_output.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip6_output.c,v 1.260 2021/07/27 17:13:03 mvs Exp $	*/
+/*	$OpenBSD: ip6_output.c,v 1.261 2021/11/24 18:48:33 bluhm Exp $	*/
 /*	$KAME: ip6_output.c,v 1.172 2001/03/25 09:55:56 itojun Exp $	*/
 
 /*
@@ -144,6 +144,9 @@ static __inline u_int16_t __attribute__((__unused__))
     u_int32_t, u_int32_t);
 void in6_delayed_cksum(struct mbuf *, u_int8_t);
 
+int ip6_output_ipsec_pmtu_update(struct tdb *, struct route_in6 *,
+    struct in6_addr *, int, int, int);
+
 /* Context for non-repeating IDs */
 struct idgen32_ctx ip6_id_ctx;
 
@@ -2771,6 +2774,51 @@ ip6_output_ipsec_lookup(struct mbuf *m, int *error, struct inpcb *inp)
 	return tdb;
 }
 
+int
+ip6_output_ipsec_pmtu_update(struct tdb *tdb, struct route_in6 *ro,
+    struct in6_addr *dst, int ifidx, int rtableid, int transportmode)
+{
+	struct rtentry *rt = NULL;
+	int rt_mtucloned = 0;
+
+	/* Find a host route to store the mtu in */
+	if (ro != NULL)
+		rt = ro->ro_rt;
+	/* but don't add a PMTU route for transport mode SAs */
+	if (transportmode)
+		rt = NULL;
+	else if (rt == NULL || (rt->rt_flags & RTF_HOST) == 0) {
+		struct sockaddr_in6 sin6;
+		int error;
+
+		memset(&sin6, 0, sizeof(sin6));
+		sin6.sin6_family = AF_INET6;
+		sin6.sin6_len = sizeof(sin6);
+		sin6.sin6_addr = *dst;
+		sin6.sin6_scope_id = in6_addr2scopeid(ifidx, dst);
+		error = in6_embedscope(dst, &sin6, NULL);
+		if (error) {
+			/* should be impossible */
+			return error;
+		}
+		rt = icmp6_mtudisc_clone(&sin6, rtableid, 1);
+		rt_mtucloned = 1;
+	}
+	DPRINTF("spi %08x mtu %d rt %p cloned %d",
+	    ntohl(tdb->tdb_spi), tdb->tdb_mtu, rt, rt_mtucloned);
+	if (rt != NULL) {
+		rt->rt_mtu = tdb->tdb_mtu;
+		if (ro != NULL && ro->ro_rt != NULL) {
+			rtfree(ro->ro_rt);
+			ro->ro_rt = rtalloc(sin6tosa(&ro->ro_dst), RT_RESOLVE,
+			    rtableid);
+		}
+		if (rt_mtucloned)
+			rtfree(rt);
+	}
+	return 0;
+}
+
 int
 ip6_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route_in6 *ro,
     int tunalready, int fwd)
@@ -2779,7 +2827,8 @@ ip6_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route_in6 *ro,
 	struct ifnet *encif;
 #endif
 	struct ip6_hdr *ip6;
-	int error;
+	struct in6_addr dst;
+	int error, ifidx, rtableid;
 
 #if NPF > 0
 	/*
@@ -2804,55 +2853,23 @@ ip6_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route_in6 *ro,
 
 	/* Check if we are allowed to fragment */
 	ip6 = mtod(m, struct ip6_hdr *);
+	dst = ip6->ip6_dst;
+	ifidx = m->m_pkthdr.ph_ifidx;
+	rtableid = m->m_pkthdr.ph_rtableid;
 	if (ip_mtudisc && tdb->tdb_mtu &&
 	    sizeof(struct ip6_hdr) + ntohs(ip6->ip6_plen) > tdb->tdb_mtu &&
 	    tdb->tdb_mtutimeout > gettime()) {
-		struct rtentry *rt = NULL;
-		int rt_mtucloned = 0;
-		int transportmode = 0;
+		int transportmode;
 
 		transportmode = (tdb->tdb_dst.sa.sa_family == AF_INET6) &&
-		    (IN6_ARE_ADDR_EQUAL(&tdb->tdb_dst.sin6.sin6_addr,
-		    &ip6->ip6_dst));
-
-		/* Find a host route to store the mtu in */
-		if (ro != NULL)
-			rt = ro->ro_rt;
-		/* but don't add a PMTU route for transport mode SAs */
-		if (transportmode)
-			rt = NULL;
-		else if (rt == NULL || (rt->rt_flags & RTF_HOST) == 0) {
-			struct sockaddr_in6 sin6;
-
-			memset(&sin6, 0, sizeof(sin6));
-			sin6.sin6_family = AF_INET6;
-			sin6.sin6_len = sizeof(sin6);
-			sin6.sin6_addr = ip6->ip6_dst;
-			sin6.sin6_scope_id =
-			    in6_addr2scopeid(m->m_pkthdr.ph_ifidx,
-			    &ip6->ip6_dst);
-			error = in6_embedscope(&ip6->ip6_dst, &sin6, NULL);
-			if (error) {
-				/* should be impossible */
-				ipsecstat_inc(ipsec_odrops);
-				m_freem(m);
-				return error;
-			}
-			rt = icmp6_mtudisc_clone(&sin6,
-			    m->m_pkthdr.ph_rtableid, 1);
-			rt_mtucloned = 1;
-		}
-		DPRINTF("spi %08x mtu %d rt %p cloned %d",
-		    ntohl(tdb->tdb_spi), tdb->tdb_mtu, rt, rt_mtucloned);
-		if (rt != NULL) {
-			rt->rt_mtu = tdb->tdb_mtu;
-			if (ro != NULL && ro->ro_rt != NULL) {
-				rtfree(ro->ro_rt);
-				ro->ro_rt = rtalloc(sin6tosa(&ro->ro_dst),
-				    RT_RESOLVE, m->m_pkthdr.ph_rtableid);
-			}
-			if (rt_mtucloned)
-				rtfree(rt);
+		    (IN6_ARE_ADDR_EQUAL(&tdb->tdb_dst.sin6.sin6_addr, &dst));
+		error = ip6_output_ipsec_pmtu_update(tdb, ro, &dst, ifidx,
+		    rtableid, transportmode);
+		if (error) {
+			ipsecstat_inc(ipsec_odrops);
+			tdb->tdb_odrops++;
+			m_freem(m);
+			return error;
 		}
 		ipsec_adjust_mtu(m, tdb->tdb_mtu);
 		m_freem(m);
@@ -2874,6 +2891,8 @@ ip6_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route_in6 *ro,
 		ipsecstat_inc(ipsec_odrops);
 		tdb->tdb_odrops++;
 	}
+	if (ip_mtudisc && error == EMSGSIZE)
+		ip6_output_ipsec_pmtu_update(tdb, ro, &dst, ifidx, rtableid, 0);
 	return error;
 }
 #endif /* IPSEC */