HBSD MFC: Fix cleanup race between unp_dispose and unp_gc - by cem@

unp_dispose and unp_gc could race to teardown the same mbuf chains, which can lead to dereferencing freed filedesc pointers. This patch adds an IGNORE_RIGHTS flag on unpcbs marking the unpcb's RIGHTS as invalid/freed. The flag is protected by UNP_LIST_LOCK. To serialize against unp_gc, unp_dispose needs the socket object. Change the dom_dispose() KPI to take a socket object instead of an mbuf chain directly. PR: 194264 Differential Revision: https://reviews.freebsd.org/D3044 Reviewed by: mjg (earlier version) Approved by: markj (mentor) Obtained from: mjg MFC after: 1 month Sponsored by: EMC / Isilon Storage Division This commit was never MFCd to 10-STABLE, and the issue is still reproducible in 2016, with the linked test program from FreeBSD's phabricator. --8<-- Unread portion of the kernel message buffer: [206] [206] [206] Fatal trap 9: general protection fault while in kernel mode [206] cpuid = 1; apic id = 01 [206] instruction pointer = 0x20:0xffffffff809e10e8 [206] stack pointer = 0x28:0xfffffe002bd96960 [206] frame pointer = 0x28:0xfffffe002bd969e0 [206] code segment = base 0x0, limit 0xfffff, type 0x1b [206] = DPL 0, pres 1, long 1, def32 0, gran 1 [206] processor eflags = interrupt enabled, resume, IOPL = 0 [206] current process = 0 (thread taskq) [206] trap number = 9 [206] panic: general protection fault [206] cpuid = 1 [206] KDB: stack backtrace: [206] #0 0xffffffff8098dc90 at kdb_backtrace+0x60 [206] #1 0xffffffff80953ed6 at vpanic+0x126 [206] #2 0xffffffff80953f63 at panic+0x43 [206] #3 0xffffffff80d6b2cb at trap_fatal+0x36b [206] #4 0xffffffff80d6af49 at trap+0x839 [206] #5 0xffffffff80d4f3ec at calltrap+0x8 [206] #6 0xffffffff809a2940 at taskqueue_run_locked+0xf0 [206] #7 0xffffffff809a32ab at taskqueue_thread_loop+0x9b [206] #8 0xffffffff8091c144 at fork_exit+0x84 [206] #9 0xffffffff80d4f92e at fork_trampoline+0xe [206] Uptime: 3m26s [206] Dumping 73 out of 487 MB:..22%..44%..66%..88% --8<-- (cherry picked from commit 576619e) Signed-off-by: Oliver Pinter <[email protected]> CC: Bryan Drewery <[email protected]> CC: Mark Johnston <[email protected]>
HardenedBSD · Aug 8, 2016 · e19f452 · e19f452
1 parent 8900a02
commit e19f452
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 15 deletions.
diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c
@@ -742,7 +742,7 @@ sofree(struct socket *so)
 
 	VNET_SO_ASSERT(so);
 	if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL)
-		(*pr->pr_domain->dom_dispose)(so->so_rcv.sb_mb);
+		(*pr->pr_domain->dom_dispose)(so);
 	if (pr->pr_usrreqs->pru_detach != NULL)
 		(*pr->pr_usrreqs->pru_detach)(so);
 
@@ -2290,7 +2290,7 @@ sorflush(struct socket *so)
 {
 	struct sockbuf *sb = &so->so_rcv;
 	struct protosw *pr = so->so_proto;
-	struct sockbuf asb;
+	struct socket aso;
 
 	VNET_SO_ASSERT(so);
 
@@ -2315,21 +2315,22 @@ sorflush(struct socket *so)
 	 * and mutex data unchanged.
 	 */
 	SOCKBUF_LOCK(sb);
-	bzero(&asb, offsetof(struct sockbuf, sb_startzero));
-	bcopy(&sb->sb_startzero, &asb.sb_startzero,
+	bzero(&aso, sizeof(aso));
+	aso.so_pcb = so->so_pcb;
+	bcopy(&sb->sb_startzero, &aso.so_rcv.sb_startzero,
 	    sizeof(*sb) - offsetof(struct sockbuf, sb_startzero));
 	bzero(&sb->sb_startzero,
 	    sizeof(*sb) - offsetof(struct sockbuf, sb_startzero));
 	SOCKBUF_UNLOCK(sb);
 	sbunlock(sb);
 
 	/*
-	 * Dispose of special rights and flush the socket buffer.  Don't call
-	 * any unsafe routines (that rely on locks being initialized) on asb.
+	 * Dispose of special rights and flush the copied socket.  Don't call
+	 * any unsafe routines (that rely on locks being initialized) on aso.
 	 */
 	if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL)
-		(*pr->pr_domain->dom_dispose)(asb.sb_mb);
-	sbrelease_internal(&asb, so);
+		(*pr->pr_domain->dom_dispose)(&aso);
+	sbrelease_internal(&aso.so_rcv, so);
 }
 
 /*

diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
@@ -278,6 +278,7 @@ static int	unp_connectat(int, struct socket *, struct sockaddr *,
 static int	unp_connect2(struct socket *so, struct socket *so2, int);
 static void	unp_disconnect(struct unpcb *unp, struct unpcb *unp2);
 static void	unp_dispose(struct mbuf *);
+static void	unp_dispose_so(struct socket *so);
 static void	unp_shutdown(struct unpcb *);
 static void	unp_drop(struct unpcb *, int);
 static void	unp_gc(__unused void *, int);
@@ -334,7 +335,7 @@ static struct domain localdomain = {
 	.dom_name =		"local",
 	.dom_init =		unp_init,
 	.dom_externalize =	unp_externalize,
-	.dom_dispose =		unp_dispose,
+	.dom_dispose =		unp_dispose_so,
 	.dom_protosw =		localsw,
 	.dom_protoswNPROTOSW =	&localsw[sizeof(localsw)/sizeof(localsw[0])]
 };
@@ -2188,15 +2189,19 @@ unp_gc_process(struct unpcb *unp)
 	 * Mark all sockets we reference with RIGHTS.
 	 */
 	so = unp->unp_socket;
-	SOCKBUF_LOCK(&so->so_rcv);
-	unp_scan(so->so_rcv.sb_mb, unp_accessable);
-	SOCKBUF_UNLOCK(&so->so_rcv);
+	if ((unp->unp_gcflag & UNPGC_IGNORE_RIGHTS) == 0) {
+		SOCKBUF_LOCK(&so->so_rcv);
+		unp_scan(so->so_rcv.sb_mb, unp_accessable);
+		SOCKBUF_UNLOCK(&so->so_rcv);
+	}
 
 	/*
 	 * Mark all sockets in our accept queue.
 	 */
 	ACCEPT_LOCK();
 	TAILQ_FOREACH(soa, &so->so_comp, so_list) {
+		if ((sotounpcb(soa)->unp_gcflag & UNPGC_IGNORE_RIGHTS) != 0)
+			continue;
 		SOCKBUF_LOCK(&soa->so_rcv);
 		unp_scan(soa->so_rcv.sb_mb, unp_accessable);
 		SOCKBUF_UNLOCK(&soa->so_rcv);
@@ -2226,11 +2231,13 @@ unp_gc(__unused void *arg, int pending)
 	unp_taskcount++;
 	UNP_LIST_LOCK();
 	/*
-	 * First clear all gc flags from previous runs.
+	 * First clear all gc flags from previous runs, apart from
+	 * UNPGC_IGNORE_RIGHTS.
 	 */
 	for (head = heads; *head != NULL; head++)
 		LIST_FOREACH(unp, *head, unp_link)
-			unp->unp_gcflag = 0;
+			unp->unp_gcflag =
+			    (unp->unp_gcflag & UNPGC_IGNORE_RIGHTS);
 
 	/*
 	 * Scan marking all reachable sockets with UNPGC_REF.  Once a socket
@@ -2307,6 +2314,21 @@ unp_dispose(struct mbuf *m)
 		unp_scan(m, unp_freerights);
 }
 
+/*
+ * Synchronize against unp_gc, which can trip over data as we are freeing it.
+ */
+static void
+unp_dispose_so(struct socket *so)
+{
+	struct unpcb *unp;
+
+	unp = sotounpcb(so);
+	UNP_LIST_LOCK();
+	unp->unp_gcflag |= UNPGC_IGNORE_RIGHTS;
+	UNP_LIST_UNLOCK();
+	unp_dispose(so->so_rcv.sb_mb);
+}
+
 static void
 unp_scan(struct mbuf *m0, void (*op)(struct filedescent **, int))
 {

diff --git a/sys/sys/domain.h b/sys/sys/domain.h
@@ -42,6 +42,7 @@
  */
 struct	mbuf;
 struct	ifnet;
+struct	socket;
 
 struct domain {
 	int	dom_family;		/* AF_xxx */
@@ -53,7 +54,7 @@ struct domain {
 	int	(*dom_externalize)	/* externalize access rights */
 		(struct mbuf *, struct mbuf **, int);
 	void	(*dom_dispose)		/* dispose of internalized rights */
-		(struct mbuf *);
+		(struct socket *);
 	struct	protosw *dom_protosw, *dom_protoswNPROTOSW;
 	struct	domain *dom_next;
 	int	(*dom_rtattach)		/* initialize routing table */

diff --git a/sys/sys/unpcb.h b/sys/sys/unpcb.h
@@ -106,6 +106,7 @@ struct unpcb {
 #define	UNPGC_REF			0x1	/* unpcb has external ref. */
 #define	UNPGC_DEAD			0x2	/* unpcb might be dead. */
 #define	UNPGC_SCANNED			0x4	/* Has been scanned. */
+#define	UNPGC_IGNORE_RIGHTS		0x8	/* Attached rights are freed */
 
 /*
  * These flags are used to handle non-atomicity in connect() and bind()