DF-2447 / fix.diff
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 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 | 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; } |