DragonFlyBSD Kernel Audit
DF-2447 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/dev/disk/dm/dm_dev.c b/sys/dev/disk/dm/dm_dev.c
--- a/sys/dev/disk/dm/dm_dev.c
+++ b/sys/dev/disk/dm/dm_dev.c
@@ -300,6 +300,10 @@
 
 /*
  * dm_dev_remove is called to completely destroy & remove a dm disk device.
+ *
+ * NOTE: the caller MUST NOT hold a busy reference on @dmv. disable_dev()
+ * waits for ref_cnt to drop to zero, so any caller-held reference would
+ * deadlock it.
  */
 int
 dm_dev_remove(dm_dev_t *dmv)
@@ -313,6 +317,49 @@
 	dm_dev_destroy(dmv);
 
 	return 0;
+}
+
+/*
+ * Atomically look a device up by name/uuid/minor, verify it is not open,
+ * remove it from the global list, wait for in-flight references to drain,
+ * and destroy it.
+ *
+ * The lookup, the is_open check and the removal from dm_dev_list all happen
+ * under dm_dev_mutex, so two concurrent removers are serialized: the first
+ * one removes and frees the device, the second one finds nothing (ENOENT).
+ * This closes the use-after-free window that existed when dm_dev_remove_ioctl
+ * dropped its busy reference (dm_dev_unbusy) between the is_open check and the
+ * call to dm_dev_remove(): in that window a concurrent remover could free the
+ * very same dm_dev_t out from under the first remover.
+ *
+ * Unlike dm_dev_remove(), this helper performs the lookup itself and never
+ * takes a long-lived busy reference, so disable_dev()'s wait-for-refcnt-zero
+ * can complete.
+ */
+int
+dm_dev_destroy_by_key(const char *name, const char *uuid, int minor)
+{
+	dm_dev_t *dmv;
+
+	lockmgr(&dm_dev_mutex, LK_EXCLUSIVE);
+
+	if ((dmv = _dm_dev_lookup(name, uuid, minor)) == NULL) {
+		lockmgr(&dm_dev_mutex, LK_RELEASE);
+		return ENOENT;
+	}
+
+	if (dmv->is_open) {
+		lockmgr(&dm_dev_mutex, LK_RELEASE);
+		return EBUSY;
+	}
+
+	/* disable_dev() removes dmv from dm_dev_list and waits for ref_cnt==0. */
+	disable_dev(dmv);
+	lockmgr(&dm_dev_mutex, LK_RELEASE);
+
+	dm_dev_destroy(dmv);
+
+	return 0;
 }
 
 int
diff --git a/sys/dev/disk/dm/dm.h b/sys/dev/disk/dm/dm.h
--- a/sys/dev/disk/dm/dm.h
+++ b/sys/dev/disk/dm/dm.h
@@ -238,6 +238,7 @@
 int dm_dev_create(dm_dev_t **, const char *, const char *, int);
 int dm_dev_remove(dm_dev_t *);
 int dm_dev_remove_all(int);
+int dm_dev_destroy_by_key(const char *, const char *, int);
 void dm_dev_busy(dm_dev_t *);
 void dm_dev_unbusy(dm_dev_t *);
 prop_array_t dm_dev_prop_list(void);
diff --git a/sys/dev/disk/dm/dm_ioctl.c b/sys/dev/disk/dm/dm_ioctl.c
--- a/sys/dev/disk/dm/dm_ioctl.c
+++ b/sys/dev/disk/dm/dm_ioctl.c
@@ -330,9 +330,9 @@
 int
 dm_dev_remove_ioctl(prop_dictionary_t dm_dict)
 {
-	dm_dev_t *dmv;
 	const char *name, *uuid;
-	uint32_t flags, minor, is_open;
+	uint32_t flags, minor;
+	int error;
 
 	flags = 0;
 	name = NULL;
@@ -346,19 +346,20 @@
 
 	dm_dbg_print_flags(flags);
 
-	if ((dmv = dm_dev_lookup(name, uuid, minor)) == NULL) {
+	/*
+	 * Atomically look the device up, check that it is not open, remove it
+	 * from the global list, drain in-flight references and destroy it -- all
+	 * under dm_dev_mutex inside dm_dev_destroy_by_key().  We must NOT take a
+	 * busy reference and drop it before the destroy: doing so opened a window
+	 * (dm_dev_unbusy here, then dm_dev_remove(dmv)) in which a concurrent
+	 * remover could free the same dm_dev_t out from under us -- a
+	 * use-after-free (DF-2447).
+	 */
+	error = dm_dev_destroy_by_key(name, uuid, minor);
+	if (error == ENOENT)
 		dm_remove_flag(dm_dict, &flags, DM_EXISTS_FLAG);
-		return ENOENT;
-	}
 
-	is_open = dmv->is_open;
-
-	dm_dev_unbusy(dmv);
-
-	if (is_open)
-		return EBUSY;
-
-	return dm_dev_remove(dmv);
+	return error;
 }
 
 /*
@@ -515,11 +516,16 @@
 	prop_dictionary_set_uint32(dm_dict, DM_IOCTL_FLAGS, flags);
 	prop_dictionary_set_uint32(dm_dict, DM_IOCTL_MINOR, dmv->minor);
 
-	dm_dev_unbusy(dmv);
-
-	/* Destroy inactive table after resume. */
+	/*
+	 * Destroy the inactive table BEFORE dropping our busy reference.
+	 * dm_table_destroy() dereferences dmv->table_head; if we unbusied first
+	 * (dropping the last reference), a concurrent remover could free dmv out
+	 * from under us -- a use-after-free (DF-2447).
+	 */
 	dm_table_destroy(&dmv->table_head, DM_TABLE_INACTIVE);
 
+	dm_dev_unbusy(dmv);
+
 	return 0;
 }