DragonFlyBSD Kernel Audit
DF-0745 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/netbt/l2cap_misc.c b/sys/netbt/l2cap_misc.c
index 0000000..1111111 100644
--- a/sys/netbt/l2cap_misc.c
+++ b/sys/netbt/l2cap_misc.c
@@ -165,9 +165,24 @@
 {
 	struct hci_link *link = req->lr_link;
 
-	callout_stop(&req->lr_rtx);
-	if (callout_active(&req->lr_rtx))
-		return;
+	/*
+	 * Synchronously drain any in-progress RTX callback before unlinking
+	 * and freeing req.  The previous code used callout_stop() plus a
+	 * callout_active() guard, but that guard was dead code: callout_stop()
+	 * (and callout_drain()) unconditionally clears CALLOUT_ACTIVE in
+	 * _callout_cancel_or_stop() (kern_timeout.c), so the early return
+	 * never fired.  As a result l2cap_request_free() always fell through
+	 * to the TAILQ_REMOVE/zfree even when the l2cap_rtx() callback was
+	 * still in progress on another CPU, which raced the callback's own
+	 * free of req and caused a double-free / TAILQ corruption.
+	 *
+	 * callout_drain() blocks until a running callback has returned
+	 * (recursing from within the callback itself returns immediately),
+	 * so an external caller can no longer race the callback.  Combined
+	 * with l2cap_rtx() no longer freeing req itself, req is now freed
+	 * exactly once.
+	 */
+	callout_drain(&req->lr_rtx);
 
 	TAILQ_REMOVE(&link->hl_reqs, req, lr_next);
 	zfree(l2cap_req_pool, req);
@@ -185,15 +200,27 @@
 {
 	struct l2cap_req *req = arg;
 	struct l2cap_channel *chan;
+	uint8_t id;
 
+	/*
+	 * The callback must not free req itself.  l2cap_request_free() now
+	 * callout_drain()s lr_rtx; calling it from inside the callback would
+	 * recurse, and -- more importantly -- freeing req from the callback
+	 * raced any concurrent external caller of l2cap_request_free() and
+	 * produced a double-free.  Capture the fields we still need (the old
+	 * code also dereferenced req->lr_id in the DPRINTF *after* freeing
+	 * req, a latent use-after-free), then let the channel close path
+	 * clean up: l2cap_close() sweeps hl_reqs and frees any request whose
+	 * lr_chan matches (l2cap_lower.c), so req is freed exactly once from
+	 * a single context.
+	 */
 	chan = req->lr_chan;
-	l2cap_request_free(req);
+	id = req->lr_id;
 
-	DPRINTF("cid %d, ident %d\n", (chan ? chan->lc_lcid : 0), req->lr_id);
+	DPRINTF("cid %d, ident %d\n", (chan ? chan->lc_lcid : 0), id);
 
 	if (chan && chan->lc_state != L2CAP_CLOSED)
 		l2cap_close(chan, ETIMEDOUT);
-
 }
 
 /*