⬢ DragonFlyBSD Kernel Audit
DF-5749 / fix.diff
← back to finding ↓ download raw
--- a/sys/netbt/l2cap.h
@@ -355,12 +355,16 @@
 
 #ifdef _KERNEL
 #include <vm/vm_zone.h>
+#include <sys/spinlock.h>
 
 #include <net/if.h>		/* for struct ifqueue */
 
 LIST_HEAD(l2cap_channel_list, l2cap_channel);
 
 /* global variables */
+extern struct spinlock		l2cap_lock;	/* protects the request
+						 * lists / channel close
+						 * claim (DF-5749) */
 extern struct l2cap_channel_list l2cap_active_list;
 extern struct l2cap_channel_list l2cap_listen_list;
 extern vm_zone_t l2cap_pdu_pool;
@@ -416,6 +420,8 @@
 #define L2CAP_SHUTDOWN		(1<<0)	/* channel is closing */
 #define L2CAP_WAIT_CONFIG_REQ	(1<<1)	/* waiting for config request */
 #define L2CAP_WAIT_CONFIG_RSP	(1<<2)	/* waiting for config response */
+#define L2CAP_CLOSING		(1<<3)	/* close body in progress
+					 * (see l2cap_close, DF-5749) */
 
 /*
  * L2CAP Request
--- a/sys/netbt/l2cap_misc.c
@@ -36,6 +36,7 @@
 #include <sys/mbuf.h>
 #include <sys/proc.h>
 #include <sys/queue.h>
+#include <sys/spinlock2.h>
 #include <sys/systm.h>
 #include <net/if.h>
 
@@ -43,6 +44,18 @@
 #include <netbt/hci.h>
 #include <netbt/l2cap.h>
 
+/*
+ * DF-5749: the L2CAP request lists and the channel close transition are
+ * touched from the softclock thread (l2cap_rtx), the netisr protocol
+ * port (socket close / signal processing) and, on hci_link teardown,
+ * the event context -- with no serialization in the original code.
+ * l2cap_lock makes the request-list mutations and the single execution
+ * of the l2cap_close() body atomic.  It is NEVER held across
+ * callout_stop() (which synchronously drains an in-flight l2cap_rtx()
+ * that may be spinning on this lock) nor across hci_* calls.
+ */
+struct spinlock l2cap_lock = SPINLOCK_INITIALIZER(l2cap_lock, 0);
+
 struct l2cap_channel_list
 	l2cap_active_list = LIST_HEAD_INITIALIZER(l2cap_active_list);
 struct l2cap_channel_list
@@ -136,7 +149,11 @@
 	callout_reset(&req->lr_rtx, l2cap_response_timeout * hz,
 	    l2cap_rtx, req);
 
+	/* list insertion only under l2cap_lock; the callout is armed
+	 * first so we never nest callout-internal spins inside it */
+	spin_lock(&l2cap_lock);
 	TAILQ_INSERT_TAIL(&link->hl_reqs, req, lr_next);
+	spin_unlock(&l2cap_lock);
 
 	return 0;
 }
@@ -163,13 +180,37 @@
 void
 l2cap_request_free(struct l2cap_req *req)
 {
-	struct hci_link *link = req->lr_link;
+	/*
+	 * DF-5749: callout_stop() synchronously drains an in-flight
+	 * l2cap_rtx() callback before returning (kern_timeout.c), so the
+	 * original "if (callout_active()) return" deferral was dead code
+	 * and concurrent callers ran the TAILQ_REMOVE/zfree twice.
+	 *
+	 * Ownership is claimed BEFORE the drain: the test-and-clear of
+	 * lr_chan under l2cap_lock decides exactly one caller as the
+	 * owner.  The owner then drains the callback -- an in-flight
+	 * l2cap_rtx() reads lr_chan at its start, sees NULL (we cleared
+	 * it) and does nothing: our own channel close subsumes the
+	 * timeout teardown -- and finally pulls the request off the
+	 * link list and frees it.  Reading the request AFTER the drain
+	 * is not possible safely: the callback would have freed and
+	 * reused the request meanwhile (the vm_zone freelist linkage
+	 * overlays lr_link/lr_chan).
+	 */
+	spin_lock(&l2cap_lock);
+	if (req->lr_chan == NULL) {
+		spin_unlock(&l2cap_lock);
+		return;
+	}
+	req->lr_chan = NULL;
+	spin_unlock(&l2cap_lock);
 
 	callout_stop(&req->lr_rtx);
-	if (callout_active(&req->lr_rtx))
-		return;
 
-	TAILQ_REMOVE(&link->hl_reqs, req, lr_next);
+	spin_lock(&l2cap_lock);
+	TAILQ_REMOVE(&req->lr_link->hl_reqs, req, lr_next);
+	spin_unlock(&l2cap_lock);
+
 	zfree(l2cap_req_pool, req);
 }
 
@@ -186,8 +227,15 @@
 	struct l2cap_req *req = arg;
 	struct l2cap_channel *chan;
 
+	/*
+	 * DF-5749: the callback no longer frees its own request -- any
+	 * concurrent l2cap_request_free() on this request blocks in its
+	 * callout_stop() until this callback returns, and then owns the
+	 * removal/free exclusively.  The channel close (whose request
+	 * walk frees this request) re-checks lc_state under l2cap_lock
+	 * so the body executes exactly once.
+	 */
 	chan = req->lr_chan;
-	l2cap_request_free(req);
 
 	DPRINTF("cid %d, ident %d\n", (chan ? chan->lc_lcid : 0), req->lr_id);
 
--- a/sys/netbt/l2cap_lower.c
@@ -38,6 +38,7 @@
 #include <sys/mbuf.h>
 #include <sys/proc.h>
 #include <sys/queue.h>
+#include <sys/spinlock2.h>
 #include <sys/systm.h>
 #include <sys/endian.h>
 
@@ -65,25 +66,53 @@
 {
 	struct l2cap_pdu *pdu;
 	struct l2cap_req *req, *n;
+	const struct btproto *proto;
+	void *upper;
+	struct hci_link *link;
 
 	if (chan->lc_state == L2CAP_CLOSED)
 		return;
 
 	/*
-	 * Since any potential PDU could be half sent we just let it go,
-	 * but disassociate ourselves from it as links deal with ownerless
-	 * PDU's in any case.  We could try harder to flush unsent packets
-	 * but maybe its better to leave them in the queue?
+	 * DF-5749 single-execution claim: softclock (l2cap_rtx), the
+	 * netisr protocol port (socket close / signal handlers) and the
+	 * hci event context can all reach this function on the same
+	 * channel with no serialization in the original code, running
+	 * the body twice (hci_acl_close(NULL) page fault, double free
+	 * of the ACL link and of the request).  Claiming L2CAP_CLOSED
+	 * under l2cap_lock BEFORE touching lc_link guarantees exactly
+	 * one body execution; losers return immediately.
 	 */
-	TAILQ_FOREACH(pdu, &chan->lc_link->hl_txq, lp_next) {
-		if (pdu->lp_chan == chan)
-			pdu->lp_chan = NULL;
+	spin_lock(&l2cap_lock);
+	if (chan->lc_state == L2CAP_CLOSED) {
+		/*
+		 * Another context is executing (or has executed) the
+		 * close body.  Wait until the L2CAP_CLOSING flag clears
+		 * so that once l2cap_close() returns, no context is
+		 * touching the channel -- l2cap_detach() kfrees it.
+		 */
+		while (chan->lc_flags & L2CAP_CLOSING) {
+			spin_unlock(&l2cap_lock);
+			cpu_ccfence();
+			spin_lock(&l2cap_lock);
+		}
+		spin_unlock(&l2cap_lock);
+		return;
 	}
+	chan->lc_state = L2CAP_CLOSED;
+	chan->lc_flags |= L2CAP_CLOSING;
+	link = chan->lc_link;			/* exclusive from here */
+	proto = chan->lc_proto;
+	upper = chan->lc_upper;
+	spin_unlock(&l2cap_lock);
 
 	/*
-	 * and clear any outstanding requests..
+	 * Clear any outstanding requests without l2cap_lock held:
+	 * l2cap_request_free() calls callout_stop() which synchronously
+	 * drains an in-flight l2cap_rtx() callback, and that callback
+	 * needs l2cap_lock for its own l2cap_close() claim above.
 	 */
-	req = TAILQ_FIRST(&chan->lc_link->hl_reqs);
+	req = TAILQ_FIRST(&link->hl_reqs);
 	while (req != NULL) {
 		n = TAILQ_NEXT(req, lr_next);
 		if (req->lr_chan == chan)
@@ -92,12 +121,29 @@
 		req = n;
 	}
 
+	/*
+	 * Since any potential PDU could be half sent we just let it go,
+	 * but disassociate ourselves from it as links deal with ownerless
+	 * PDU's in any case.  We could try harder to flush unsent packets
+	 * but maybe its better to leave them in the queue?
+	 */
+	TAILQ_FOREACH(pdu, &link->hl_txq, lp_next) {
+		if (pdu->lp_chan == chan)
+			pdu->lp_chan = NULL;
+	}
+
 	chan->lc_pending = 0;
-	chan->lc_state = L2CAP_CLOSED;
-	hci_acl_close(chan->lc_link, err);
 	chan->lc_link = NULL;
 
-	(*chan->lc_proto->disconnected)(chan->lc_upper, err);
+	/* outside l2cap_lock: hci_acl_close() can reach hci_link_free()
+	 * which closes other channels on the same link */
+	hci_acl_close(link, err);
+
+	(*proto->disconnected)(upper, err);
+
+	/* body complete: wake any closer waiting on L2CAP_CLOSING
+	 * (plain store: this context owns the body exclusively) */
+	chan->lc_flags &= ~L2CAP_CLOSING;
 }
 
 /*
--- a/sys/netbt/l2cap_upper.c