From 0d8b755c1cb3551294d5fa172254074877ce1c65 Mon Sep 17 00:00:00 2001 From: Matthew Ahrens Date: Wed, 4 Dec 2019 15:18:45 -0800 Subject: [PATCH] fix use-after-free of vd_path in spa_vdev_remove() 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 0x608000a1fcd0 at pc 0x7fe88b0c166e bp 0x7fe878414ad0 sp 0x7fe878414278 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 ../../module/zfs/spa_misc.c:1447 #2 0x7fe88a688034 in spa_vdev_remove ../../module/zfs/vdev_removal.c:2259 #3 0x55ffbc7748f8 in ztest_vdev_aux_add_remove /export/home/delphix/zfs/cmd/ztest/ztest.c:3229 #4 0x55ffbc769fba in ztest_execute /export/home/delphix/zfs/cmd/ztest/ztest.c:6714 #5 0x55ffbc779a90 in ztest_thread /export/home/delphix/zfs/cmd/ztest/ztest.c:6761 #6 0x7fe889cbc6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da) #7 0x7fe8899e588e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x12188e) 0x608000a1fcd0 is located 48 bytes inside of 88-byte region [0x608000a1fca0,0x608000a1fcf8) freed by thread T686 here: #0 0x7fe88b14e7b8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7b8) #1 0x7fe88ae541c5 in nvlist_free ../../module/nvpair/nvpair.c:874 #2 0x7fe88ae543ba in nvpair_free ../../module/nvpair/nvpair.c:844 #3 0x7fe88ae57400 in nvlist_remove_nvpair ../../module/nvpair/nvpair.c:978 #4 0x7fe88a683c81 in spa_vdev_remove_aux ../../module/zfs/vdev_removal.c:185 #5 0x7fe88a68857c in spa_vdev_remove ../../module/zfs/vdev_removal.c:2221 #6 0x55ffbc7748f8 in ztest_vdev_aux_add_remove /export/home/delphix/zfs/cmd/ztest/ztest.c:3229 #7 0x55ffbc769fba in ztest_execute /export/home/delphix/zfs/cmd/ztest/ztest.c:6714 #8 0x55ffbc779a90 in ztest_thread /export/home/delphix/zfs/cmd/ztest/ztest.c:6761 #9 0x7fe889cbc6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da) Signed-off-by: Matthew Ahrens --- module/zfs/vdev_removal.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/module/zfs/vdev_removal.c b/module/zfs/vdev_removal.c index 8089571e12e2..202b7aa37d72 100644 --- a/module/zfs/vdev_removal.c +++ b/module/zfs/vdev_removal.c @@ -2166,7 +2166,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)); @@ -2201,7 +2201,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); @@ -2214,7 +2215,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. */ @@ -2227,7 +2229,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); @@ -2239,9 +2242,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) @@ -2254,12 +2254,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);