Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 08 Jun 2026 14:34:26 +0000
From:      Adrian Chadd <adrian@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Cc:        Abdelkader Boudih <freebsd@seuros.com>
Subject:   git: a9519f7821c0 - main - firewire: Fix watchdog_clock aliasing and fw_tl2xfer UAF race
Message-ID:  <6a26d2f2.46f84.4c1d12e1@gitrepo.freebsd.org>
index | next in thread | raw e-mail 

The branch main has been updated by adrian:

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

commit a9519f7821c066c393690603eab33043f3804a0c
Author:     Abdelkader Boudih <freebsd@seuros.com>
AuthorDate: 2026-06-08 14:30:29 +0000
Commit:     Adrian Chadd <adrian@FreeBSD.org>
CommitDate: 2026-06-08 14:30:29 +0000

    firewire: Fix watchdog_clock aliasing and fw_tl2xfer UAF race
    
    Two bugs in the firewire bus layer that affect all consumers (
    if_fwip, sbp):
    
    watchdog_clock was a static local in firewire_watchdog(), shared across
    all firewire_comm instances.  With two controllers (e.g. built-in +
    Thunderbolt Display), both advance the same counter, so the second
    controller's 15-second boot-time timeout guard expires prematurely.
    
    fw_tl2xfer() released tlabel_lock before returning the xfer pointer.
    
    Reviewed by:    zlei, adrian
    Differential Revision:  https://reviews.freebsd.org/D57496
---
 sys/dev/firewire/firewire.c    | 67 +++++++++++++++++++++++-------------------
 sys/dev/firewire/firewirereg.h |  1 +
 2 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/sys/dev/firewire/firewire.c b/sys/dev/firewire/firewire.c
index 079a50413ffa..a54cbf9cdf0b 100644
--- a/sys/dev/firewire/firewire.c
+++ b/sys/dev/firewire/firewire.c
@@ -372,23 +372,21 @@ firewire_xfer_timeout(void *arg, int pending)
 static void
 firewire_watchdog(void *arg)
 {
-	struct firewire_comm *fc;
-	static int watchdog_clock = 0;
-
-	fc = arg;
+	struct firewire_softc *sc = arg;
+	struct firewire_comm *fc = sc->fc;
 
 	/*
 	 * At boot stage, the device interrupt is disabled and
-	 * We encounter a timeout easily. To avoid this,
-	 * ignore clock interrupt for a while.
+	 * we encounter a timeout easily. To avoid this,
+	 * ignore clock ticks for a while.
 	 */
-	if (watchdog_clock > WATCHDOG_HZ * 15)
+	if (sc->watchdog_clock > WATCHDOG_HZ * 15)
 		taskqueue_enqueue(fc->taskqueue, &fc->task_timeout);
 	else
-		watchdog_clock++;
+		sc->watchdog_clock++;
 
 	callout_reset(&fc->timeout_callout, hz / WATCHDOG_HZ,
-	    firewire_watchdog, fc);
+	    firewire_watchdog, sc);
 }
 
 /*
@@ -444,8 +442,9 @@ firewire_attach(device_t dev)
 	CALLOUT_INIT(&fc->busprobe_callout);
 	TASK_INIT(&fc->task_timeout, 0, firewire_xfer_timeout, fc);
 
+	sc->watchdog_clock = 0;
 	callout_reset(&sc->fc->timeout_callout, hz,
-	    firewire_watchdog, sc->fc);
+	    firewire_watchdog, sc);
 
 	/* create thread */
 	kproc_create(fw_bus_probe_thread, fc, &fc->probe_thread,
@@ -1048,39 +1047,47 @@ fw_tl_free(struct firewire_comm *fc, struct fw_xfer *xfer)
 }
 
 /*
- * To obtain XFER structure by transaction label.
+ * Look up an XFER by transaction label.
+ * Removes the xfer from fc->tlabels only when AT transmit has completed
+ * (FWXF_SENT); FWXF_START xfers remain so fw_drain_txq() can find them
+ * on a bus reset.
  */
 static struct fw_xfer *
 fw_tl2xfer(struct firewire_comm *fc, int node, int tlabel, int tcode)
 {
 	struct fw_xfer *xfer;
-	int s = splfw();
 	int req;
 
 	mtx_lock(&fc->tlabel_lock);
-	STAILQ_FOREACH(xfer, &fc->tlabels[tlabel], tlabel)
-		if (xfer->send.hdr.mode.hdr.dst == node) {
+	STAILQ_FOREACH(xfer, &fc->tlabels[tlabel], tlabel) {
+		if (xfer->send.hdr.mode.hdr.dst != node)
+			continue;
+		/* Validate tcode match before claiming the xfer. */
+		req = xfer->send.hdr.mode.hdr.tcode;
+		if (xfer->fc->tcode[req].valid_res != tcode) {
+			printf("%s: invalid response tcode "
+			    "(0x%x for 0x%x)\n", __func__, tcode, req);
 			mtx_unlock(&fc->tlabel_lock);
-			splx(s);
-			KASSERT(xfer->tl == tlabel,
-				("xfer->tl 0x%x != 0x%x", xfer->tl, tlabel));
-			/* extra sanity check */
-			req = xfer->send.hdr.mode.hdr.tcode;
-			if (xfer->fc->tcode[req].valid_res != tcode) {
-				printf("%s: invalid response tcode "
-				    "(0x%x for 0x%x)\n", __FUNCTION__,
-				    tcode, req);
-				return (NULL);
-			}
-
-			if (firewire_debug > 2)
-				printf("fw_tl2xfer: found tl=%d\n", tlabel);
-			return (xfer);
+			return (NULL);
 		}
+		/*
+		 * Remove from tlabels only after AT transmit completes
+		 * (FWXF_SENT).  Early responses (FWXF_START) must stay
+		 * in the list until fwohci_txd() drains the descriptor.
+		 */
+		if (xfer->flag & FWXF_SENT) {
+			STAILQ_REMOVE(&fc->tlabels[tlabel], xfer,
+			    fw_xfer, tlabel);
+			xfer->tl = -1;
+		}
+		mtx_unlock(&fc->tlabel_lock);
+		if (firewire_debug > 2)
+			printf("fw_tl2xfer: found tl=%d\n", tlabel);
+		return (xfer);
+	}
 	mtx_unlock(&fc->tlabel_lock);
 	if (firewire_debug > 1)
 		printf("fw_tl2xfer: not found tl=%d\n", tlabel);
-	splx(s);
 	return (NULL);
 }
 
diff --git a/sys/dev/firewire/firewirereg.h b/sys/dev/firewire/firewirereg.h
index d17f7a15785a..97a53606c001 100644
--- a/sys/dev/firewire/firewirereg.h
+++ b/sys/dev/firewire/firewirereg.h
@@ -69,6 +69,7 @@ struct fw_device {
 struct firewire_softc {
 	struct cdev *dev;
 	struct firewire_comm *fc;
+	int watchdog_clock;
 };
 
 #define FW_MAX_DMACH 0x20
home | help

Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a26d2f2.46f84.4c1d12e1>