Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 16 Feb 2021 22:07:39 GMT
From:      "Andrey V. Elsukov" <ae@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org
Subject:   git: 6666b1d45a1b - releng/13.0 - [udp6] fix possible panic due to lack of locking.
Message-ID:  <202102162207.11GM7dYI084234@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch releng/13.0 has been updated by ae:

URL: https://cgit.FreeBSD.org/src/commit/?id=6666b1d45a1b0ab4c31f621e6b406ff57431c91a

commit 6666b1d45a1b0ab4c31f621e6b406ff57431c91a
Author:     Andrey V. Elsukov <ae@FreeBSD.org>
AuthorDate: 2021-02-11 08:38:41 +0000
Commit:     Andrey V. Elsukov <ae@FreeBSD.org>
CommitDate: 2021-02-16 22:03:08 +0000

    [udp6] fix possible panic due to lack of locking.
    
    The lookup for a IPv6 multicast addresses corresponding to
    the destination address in the datagram is protected by the
    NET_EPOCH section. Access to each PCB is protected by INP_RLOCK
    during comparing. But access to socket's so_options field is
    not protected. And in some cases it is possible, that PCB
    pointer is still valid, but inp_socket is not. The patch wides
    lock holding to protect access to inp_socket. It copies locking
    strategy from IPv4 UDP handling.
    
    PR:     232192
    Approved by:    re (gjb)
    Obtained from:  Yandex LLC
    Sponsored by:   Yandex LLC
    Differential Revision:  https://reviews.freebsd.org/D28232
    
    (cherry picked from commit 3c782d9c91666886d868426f0aea040d1a1a8ee4)
    (cherry picked from commit 8cb4c363163062bceec92383eae43f5a32049c73)
---
 sys/netinet6/udp6_usrreq.c | 61 +++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/sys/netinet6/udp6_usrreq.c b/sys/netinet6/udp6_usrreq.c
index 1535be90e1b0..3a001fea077d 100644
--- a/sys/netinet6/udp6_usrreq.c
+++ b/sys/netinet6/udp6_usrreq.c
@@ -353,6 +353,13 @@ skip_checksum:
 					continue;
 			}
 
+			INP_RLOCK(inp);
+
+			if (__predict_false(inp->inp_flags2 & INP_FREED)) {
+				INP_RUNLOCK(inp);
+				continue;
+			}
+
 			/*
 			 * XXXRW: Because we weren't holding either the inpcb
 			 * or the hash lock when we checked for a match 
@@ -365,16 +372,10 @@ skip_checksum:
 			 * and source-specific multicast. [RFC3678]
 			 */
 			imo = inp->in6p_moptions;
-			if (imo && IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst)) {
+			if (imo != NULL) {
 				struct sockaddr_in6	 mcaddr;
 				int			 blocked;
 
-				INP_RLOCK(inp);
-				if (__predict_false(inp->inp_flags2 & INP_FREED)) {
-					INP_RUNLOCK(inp);
-					continue;
-				}
-
 				bzero(&mcaddr, sizeof(struct sockaddr_in6));
 				mcaddr.sin6_len = sizeof(struct sockaddr_in6);
 				mcaddr.sin6_family = AF_INET6;
@@ -389,33 +390,30 @@ skip_checksum:
 					if (blocked == MCAST_NOTSMEMBER ||
 					    blocked == MCAST_MUTED)
 						UDPSTAT_INC(udps_filtermcast);
-					INP_RUNLOCK(inp); /* XXX */
+					INP_RUNLOCK(inp);
 					continue;
 				}
-
-				INP_RUNLOCK(inp);
 			}
+
 			if (last != NULL) {
 				struct mbuf *n;
 
 				if ((n = m_copym(m, 0, M_COPYALL, M_NOWAIT)) !=
 				    NULL) {
-					INP_RLOCK(last);
-					if (__predict_true(last->inp_flags2 & INP_FREED) == 0) {
-						if (nxt == IPPROTO_UDPLITE)
-							UDPLITE_PROBE(receive, NULL, last,
-							    ip6, last, uh);
-						else
-							UDP_PROBE(receive, NULL, last,
-							    ip6, last, uh);
-						if (udp6_append(last, n, off, fromsa)) {
-							/* XXX-BZ do we leak m here? */
-							*mp = NULL;
-							return (IPPROTO_DONE);
-						}
+					if (nxt == IPPROTO_UDPLITE)
+						UDPLITE_PROBE(receive, NULL,
+						    last, ip6, last, uh);
+					else
+						UDP_PROBE(receive, NULL, last,
+						    ip6, last, uh);
+					if (udp6_append(last, n, off,
+					    fromsa)) {
+						INP_RUNLOCK(inp);
+						goto badunlocked;
 					}
-					INP_RUNLOCK(last);
 				}
+				/* Release PCB lock taken on previous pass. */
+				INP_RUNLOCK(last);
 			}
 			last = inp;
 			/*
@@ -441,15 +439,12 @@ skip_checksum:
 			UDPSTAT_INC(udps_noportmcast);
 			goto badunlocked;
 		}
-		INP_RLOCK(last);
-		if (__predict_true(last->inp_flags2 & INP_FREED) == 0) {
-			if (nxt == IPPROTO_UDPLITE)
-				UDPLITE_PROBE(receive, NULL, last, ip6, last, uh);
-			else
-				UDP_PROBE(receive, NULL, last, ip6, last, uh);
-			if (udp6_append(last, m, off, fromsa) == 0)
-				INP_RUNLOCK(last);
-		} else
+
+		if (nxt == IPPROTO_UDPLITE)
+			UDPLITE_PROBE(receive, NULL, last, ip6, last, uh);
+		else
+			UDP_PROBE(receive, NULL, last, ip6, last, uh);
+		if (udp6_append(last, m, off, fromsa) == 0)
 			INP_RUNLOCK(last);
 		*mp = NULL;
 		return (IPPROTO_DONE);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202102162207.11GM7dYI084234>