Skip to content

Commit

Permalink
Fix use-after-free of vd_path in spa_vdev_remove()
Browse files Browse the repository at this point in the history
After spa_vdev_remove_aux() is called, the config nvlist is no longer
valid, as it's been replaced by the new one (with the specified device
removed).  Therefore any pointers into the nvlist are no longer valid.
So we can't save the result of
`fnvlist_lookup_string(nv, ZPOOL_CONFIG_PATH)` (in vd_path) across the
call to spa_vdev_remove_aux().

Instead, use spa_strdup() to save a copy of the string before calling
spa_vdev_remove_aux.

Found by AddressSanitizer:

ERROR: AddressSanitizer: heap-use-after-free on address ...
READ of size 34 at 0x608000a1fcd0 thread T686
    #0 0x7fe88b0c166d  (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x5166d)
    #1 0x7fe88a5acd6e in spa_strdup spa_misc.c:1447
    #2 0x7fe88a688034 in spa_vdev_remove vdev_removal.c:2259
    #3 0x55ffbc7748f8 in ztest_vdev_aux_add_remove ztest.c:3229
    #4 0x55ffbc769fba in ztest_execute ztest.c:6714
    #5 0x55ffbc779a90 in ztest_thread ztest.c:6761
    #6 0x7fe889cbc6da in start_thread
    #7 0x7fe8899e588e in __clone

0x608000a1fcd0 is located 48 bytes inside of 88-byte region
freed by thread T686 here:
    #0 0x7fe88b14e7b8 in __interceptor_free
    #1 0x7fe88ae541c5 in nvlist_free nvpair.c:874
    #2 0x7fe88ae543ba in nvpair_free nvpair.c:844
    #3 0x7fe88ae57400 in nvlist_remove_nvpair nvpair.c:978
    #4 0x7fe88a683c81 in spa_vdev_remove_aux vdev_removal.c:185
    #5 0x7fe88a68857c in spa_vdev_remove vdev_removal.c:2221
    #6 0x55ffbc7748f8 in ztest_vdev_aux_add_remove ztest.c:3229
    #7 0x55ffbc769fba in ztest_execute ztest.c:6714
    #8 0x55ffbc779a90 in ztest_thread ztest.c:6761
    #9 0x7fe889cbc6da in start_thread

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes #9706
  • Loading branch information
ahrens authored and tonyhutter committed Jan 22, 2020
1 parent 4d658bd commit 856d185
Showing 1 changed file with 11 additions and 11 deletions.
22 changes: 11 additions & 11 deletions module/zfs/vdev_removal.c
Original file line number Diff line number Diff line change
Expand Up @@ -2136,7 +2136,7 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t unspare)
int error = 0, error_log;
boolean_t locked = MUTEX_HELD(&spa_namespace_lock);
sysevent_t *ev = NULL;
char *vd_type = NULL, *vd_path = NULL, *vd_path_log = NULL;
char *vd_type = NULL, *vd_path = NULL;

ASSERT(spa_writeable(spa));

Expand Down Expand Up @@ -2171,7 +2171,8 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t unspare)
ESC_ZFS_VDEV_REMOVE_AUX);

vd_type = VDEV_TYPE_SPARE;
vd_path = fnvlist_lookup_string(nv, ZPOOL_CONFIG_PATH);
vd_path = spa_strdup(fnvlist_lookup_string(
nv, ZPOOL_CONFIG_PATH));
spa_vdev_remove_aux(spa->spa_spares.sav_config,
ZPOOL_CONFIG_SPARES, spares, nspares, nv);
spa_load_spares(spa);
Expand All @@ -2184,7 +2185,8 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t unspare)
ZPOOL_CONFIG_L2CACHE, &l2cache, &nl2cache) == 0 &&
(nv = spa_nvlist_lookup_by_guid(l2cache, nl2cache, guid)) != NULL) {
vd_type = VDEV_TYPE_L2CACHE;
vd_path = fnvlist_lookup_string(nv, ZPOOL_CONFIG_PATH);
vd_path = spa_strdup(fnvlist_lookup_string(
nv, ZPOOL_CONFIG_PATH));
/*
* Cache devices can always be removed.
*/
Expand All @@ -2197,7 +2199,8 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t unspare)
} else if (vd != NULL && vd->vdev_islog) {
ASSERT(!locked);
vd_type = VDEV_TYPE_LOG;
vd_path = (vd->vdev_path != NULL) ? vd->vdev_path : "-";
vd_path = spa_strdup((vd->vdev_path != NULL) ?
vd->vdev_path : "-");
error = spa_vdev_remove_log(vd, &txg);
} else if (vd != NULL) {
ASSERT(!locked);
Expand All @@ -2209,9 +2212,6 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t unspare)
error = SET_ERROR(ENOENT);
}

if (vd_path != NULL)
vd_path_log = spa_strdup(vd_path);

error_log = error;

if (!locked)
Expand All @@ -2224,12 +2224,12 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t unspare)
* Doing that would prevent the txg sync from actually happening,
* causing a deadlock.
*/
if (error_log == 0 && vd_type != NULL && vd_path_log != NULL) {
if (error_log == 0 && vd_type != NULL && vd_path != NULL) {
spa_history_log_internal(spa, "vdev remove", NULL,
"%s vdev (%s) %s", spa_name(spa), vd_type, vd_path_log);
"%s vdev (%s) %s", spa_name(spa), vd_type, vd_path);
}
if (vd_path_log != NULL)
spa_strfree(vd_path_log);
if (vd_path != NULL)
spa_strfree(vd_path);

if (ev != NULL)
spa_event_post(ev);
Expand Down

0 comments on commit 856d185

Please sign in to comment.