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

@@ -372,23 +372,21 @@ firewire_xfer_timeout(void *arg, int pending)
 static void
 firewire_watchdog(void *arg)
 {
-	struct firewire_comm *fc;
+	struct firewire_softc *sc = arg;
-	static int watchdog_clock = 0;
+	struct firewire_comm *fc = sc->fc;
-
-	fc = arg;
 
 	/*
 	 * At boot stage, the device interrupt is disabled and
-	 * We encounter a timeout easily. To avoid this,
+	 * we encounter a timeout easily. To avoid this,
-	 * ignore clock interrupt for a while.
+	 * 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,31 +1047,40 @@ 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)
+	STAILQ_FOREACH(xfer, &fc->tlabels[tlabel], tlabel) {
-		if (xfer->send.hdr.mode.hdr.dst == node) {
+		if (xfer->send.hdr.mode.hdr.dst != node)
-			mtx_unlock(&fc->tlabel_lock);
+			continue;
-			splx(s);
+		/* Validate tcode match before claiming the xfer. */
-			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__,
+			    "(0x%x for 0x%x)\n", __func__, tcode, req);
-				    tcode, req);
+			mtx_unlock(&fc->tlabel_lock);
 			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);
@@ -1080,7 +1088,6 @@ fw_tl2xfer(struct firewire_comm *fc, int node, int tlabel, int tcode)
 	mtx_unlock(&fc->tlabel_lock);
 	if (firewire_debug > 1)
 		printf("fw_tl2xfer: not found tl=%d\n", tlabel);
-	splx(s);
 	return (NULL);
 }
 

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