⬢ DragonFlyBSD Kernel Audit
DF-4475 / fix.diff
← back to finding ↓ download raw 
 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
--- a/sys/dev/disk/nata/ata-all.c
+++ b/sys/dev/disk/nata/ata-all.c
@@ -83,6 +83,7 @@
 int ata_wc = 1;
 
 /* local vars */
+static struct lock ata_channel_ctl_lock;	/* DF-4476: serialize channel control ioctls */
 static int ata_dma = 1;
 static int atapi_dma = 1;
 
@@ -385,8 +386,10 @@
 	if (*value >= devclass_get_maxunit(ata_devclass) ||
 	    !(device = devclass_get_device(ata_devclass, *value)))
 	    return ENXIO;
+	lockmgr(&ata_channel_ctl_lock, LK_EXCLUSIVE);
 	error = ata_reinit(device);
 	ata_start(device);
+	lockmgr(&ata_channel_ctl_lock, LK_RELEASE);
 	break;
 
     case IOCATAATTACH:
@@ -394,14 +397,18 @@
 	    !(device = devclass_get_device(ata_devclass, *value)))
 	    return ENXIO;
 	/* XXX SOS should enable channel HW on controller */
+	lockmgr(&ata_channel_ctl_lock, LK_EXCLUSIVE);
 	error = ata_attach(device);
+	lockmgr(&ata_channel_ctl_lock, LK_RELEASE);
 	break;
 
     case IOCATADETACH:
 	if (*value >= devclass_get_maxunit(ata_devclass) ||
 	    !(device = devclass_get_device(ata_devclass, *value)))
 	    return ENXIO;
+	lockmgr(&ata_channel_ctl_lock, LK_EXCLUSIVE);
 	error = ata_detach(device);
+	lockmgr(&ata_channel_ctl_lock, LK_RELEASE);
 	/* XXX SOS should disable channel HW on controller */
 	break;
 
@@ -485,7 +492,17 @@
 	    request->u.ata.lba = ioc_request->u.ata.lba;
 	    request->u.ata.count = ioc_request->u.ata.count;
 	}
-	request->timeout = ioc_request->timeout;
+	/*
+	 * DF-4475: clamp the untrusted timeout.  A negative timeout makes
+	 * the ata-queue completion wait (timeout * hz * 4) wrap to a ~1-tick
+	 * expiry, so a request merely queued behind others hits the
+	 * "taskqueue timeout" fallback and is then freed by this ioctl
+	 * while the channel still owns it (use-after-free).
+	 */
+	if (ioc_request->timeout <= 0)
+	    request->timeout = ATA_DEFAULT_TIMEOUT;
+	else
+	    request->timeout = ioc_request->timeout;
 	request->data = buf;
 	request->bytecount = ioc_request->count;
 	request->transfersize = request->bytecount;
@@ -1077,6 +1094,7 @@
 					objcache_malloc_alloc,
 					objcache_malloc_free,
 					&ata_request_malloc_args);
+    lockinit(&ata_channel_ctl_lock, "atactl", 0, 0);
     ata_composite_cache = objcache_create("ata_composite", 0, 0,
 					  ata_composite_cache_ctor, NULL, NULL,
 					  objcache_malloc_alloc,
--- a/sys/dev/disk/nata/ata-queue.c
+++ b/sys/dev/disk/nata/ata-queue.c
@@ -144,10 +144,16 @@
 	    if (!(request->flags & ATA_R_COMPLETED)) {
 		/* apparently not */
 		device_printf(request->dev, "WARNING - %s taskqueue timeout - "
-			      "completing request directly\n",
+			      "waiting for channel completion\n",
 			      ata_cmd2str(request));
-		request->flags |= ATA_R_DANGER1;
-		ata_completed(request, 0);
+		/*
+		 * DF-4475: the channel (queue / running / taskqueue) may
+		 * still own this request.  Do NOT complete it here and do
+		 * NOT let the issuer free it: keep waiting; the channel's
+		 * own ata_timeout callout guarantees completion.
+		 */
+		while (!(request->flags & ATA_R_COMPLETED))
+		    lksleep(request, &request->done, 0, "atareqcw", hz);
 	    }
 	}
 	lockuninit(&request->done);