Commit 8af4b6c4 authored by Daniel Verkamp's avatar Daniel Verkamp Committed by Jim Harris
Browse files

nvmf: fix potential use-after-free in hot remove 



The subsystem->ns array may be resized with realloc(), so old ns
pointers can become invalid.

To fix this, allocate each ns as a separate object, and change the
subsystem->ns[] array to point to the namespaces rather than containing
them.

Change-Id: I873502fa90cf840e4eaa9b1abd94a95afe0f737f
Signed-off-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/399726


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
parent 168cfd12

lib/nvmf/nvmf.c

+4 −4
Original line number Diff line number Diff line
@@ -462,8 +462,8 @@ poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, 


		/* Initialize new channels */

		for (i = old_num_channels; i < new_num_channels; i++) {

			ns = &subsystem->ns[i];

			if (ns->allocated) {

			ns = subsystem->ns[i];

			if (ns) {

				sgroup->channels[i] = spdk_bdev_get_io_channel(ns->desc);

			} else {

				sgroup->channels[i] = NULL;
@@ -497,8 +497,8 @@ poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, 


		/* Initialize new channels */

		for (i = old_num_channels; i < new_num_channels; i++) {

			ns = &subsystem->ns[i];

			if (ns->allocated) {

			ns = subsystem->ns[i];

			if (ns) {

				sgroup->channels[i] = spdk_bdev_get_io_channel(ns->desc);

			} else {

				sgroup->channels[i] = NULL;

lib/nvmf/nvmf_internal.h

+3 −11
Original line number Diff line number Diff line
@@ -139,7 +139,6 @@ struct spdk_nvmf_ns { 
	struct spdk_bdev *bdev;

	struct spdk_bdev_desc *desc;

	uint32_t id;

	bool allocated;

};



struct spdk_nvmf_qpair {
@@ -194,8 +193,8 @@ struct spdk_nvmf_subsystem { 


	char sn[SPDK_NVME_CTRLR_SN_LEN + 1];



	/* Array of namespaces of size max_nsid indexed by nsid - 1 */

	struct spdk_nvmf_ns			*ns;

	/* Array of pointers to namespaces of size max_nsid indexed by nsid - 1 */

	struct spdk_nvmf_ns			**ns;

	uint32_t 				max_nsid;

	uint32_t				num_allocated_nsid;
@@ -249,19 +248,12 @@ struct spdk_nvmf_ctrlr *spdk_nvmf_subsystem_get_ctrlr(struct spdk_nvmf_subsystem 
static inline struct spdk_nvmf_ns *

_spdk_nvmf_subsystem_get_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid)

{

	struct spdk_nvmf_ns *ns;



	/* NOTE: This implicitly also checks for 0, since 0 - 1 wraps around to UINT32_MAX. */

	if (spdk_unlikely(nsid - 1 >= subsystem->max_nsid)) {

		return NULL;

	}



	ns = &subsystem->ns[nsid - 1];

	if (!ns->allocated) {

		return NULL;

	}



	return ns;

	return subsystem->ns[nsid - 1];

}



static inline bool

lib/nvmf/subsystem.c

+24 −17
Original line number Diff line number Diff line
@@ -268,7 +268,7 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt, 
	TAILQ_INIT(&subsystem->ctrlrs);



	if (num_ns != 0) {

		subsystem->ns = calloc(num_ns, sizeof(struct spdk_nvmf_ns));

		subsystem->ns = calloc(num_ns, sizeof(struct spdk_nvmf_ns *));

		if (subsystem->ns == NULL) {

			SPDK_ERRLOG("Namespace memory allocation failed\n");

			free(subsystem);
@@ -313,12 +313,12 @@ spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem) 
		spdk_nvmf_ctrlr_destruct(ctrlr);

	}



	for (ns = spdk_nvmf_subsystem_get_first_ns(subsystem); ns != NULL;

	     ns = spdk_nvmf_subsystem_get_next_ns(subsystem, ns)) {

		if (ns->bdev == NULL) {

			continue;

		}

		spdk_bdev_close(ns->desc);

	ns = spdk_nvmf_subsystem_get_first_ns(subsystem);

	while (ns != NULL) {

		struct spdk_nvmf_ns *next_ns = spdk_nvmf_subsystem_get_next_ns(subsystem, ns);



		spdk_nvmf_subsystem_remove_ns(subsystem, ns->id);

		ns = next_ns;

	}



	free(subsystem->ns);
@@ -781,13 +781,15 @@ spdk_nvmf_subsystem_remove_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t ns 
		return -1;

	}



	ns = &subsystem->ns[nsid - 1];

	if (ns->allocated == false) {

	ns = subsystem->ns[nsid - 1];

	if (!ns) {

		return -1;

	}



	subsystem->ns[nsid - 1] = NULL;



	spdk_bdev_close(ns->desc);

	ns->allocated = false;

	free(ns);

	subsystem->num_allocated_nsid--;



	return 0;
@@ -836,7 +838,7 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd 


	if (nsid > subsystem->max_nsid ||

	    (nsid == 0 && subsystem->num_allocated_nsid == subsystem->max_nsid)) {

		struct spdk_nvmf_ns *new_ns_array;

		struct spdk_nvmf_ns **new_ns_array;

		uint32_t new_max_nsid;



		if (nsid > subsystem->max_nsid) {
@@ -850,14 +852,14 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd 
			return 0;

		}



		new_ns_array = realloc(subsystem->ns, sizeof(struct spdk_nvmf_ns) * new_max_nsid);

		new_ns_array = realloc(subsystem->ns, sizeof(struct spdk_nvmf_ns *) * new_max_nsid);

		if (new_ns_array == NULL) {

			SPDK_ERRLOG("Memory allocation error while resizing namespace array.\n");

			return 0;

		}



		memset(new_ns_array + subsystem->max_nsid, 0,

		       sizeof(struct spdk_nvmf_ns) * (new_max_nsid - subsystem->max_nsid));

		       sizeof(struct spdk_nvmf_ns *) * (new_max_nsid - subsystem->max_nsid));

		subsystem->ns = new_ns_array;

		subsystem->max_nsid = new_max_nsid;

	}
@@ -882,8 +884,12 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd 
		}

	}



	ns = &subsystem->ns[nsid - 1];

	memset(ns, 0, sizeof(*ns));

	ns = calloc(1, sizeof(*ns));

	if (ns == NULL) {

		SPDK_ERRLOG("Namespace allocation failed\n");

		return 0;

	}



	ns->bdev = bdev;

	ns->id = nsid;

	ns->subsystem = subsystem;
@@ -891,9 +897,10 @@ spdk_nvmf_subsystem_add_ns(struct spdk_nvmf_subsystem *subsystem, struct spdk_bd 
	if (rc != 0) {

		SPDK_ERRLOG("Subsystem %s: bdev %s cannot be opened, error=%d\n",

			    subsystem->subnqn, spdk_bdev_get_name(bdev), rc);

		free(ns);

		return 0;

	}

	ns->allocated = true;

	subsystem->ns[nsid - 1] = ns;



	SPDK_DEBUGLOG(SPDK_LOG_NVMF, "Subsystem %s: bdev %s assigned nsid %" PRIu32 "\n",

		      spdk_nvmf_subsystem_get_nqn(subsystem),
@@ -917,7 +924,7 @@ spdk_nvmf_subsystem_get_next_allocated_nsid(struct spdk_nvmf_subsystem *subsyste 
	}



	for (nsid = prev_nsid + 1; nsid <= subsystem->max_nsid; nsid++) {

		if (subsystem->ns[nsid - 1].allocated) {

		if (subsystem->ns[nsid - 1]) {

			return nsid;

		}

	}

test/nvmf/fio/fio.sh

+5 −1
Original line number Diff line number Diff line
@@ -35,7 +35,11 @@ bdevs="$bdevs $($rpc_py construct_malloc_bdev $MALLOC_BDEV_SIZE $MALLOC_BLOCK_SI 


modprobe -v nvme-rdma



$rpc_py construct_nvmf_subsystem nqn.2016-06.io.spdk:cnode1 "trtype:RDMA traddr:$NVMF_FIRST_TARGET_IP trsvcid:4420" "" -a -s SPDK00000000000001 -n "$bdevs"

$rpc_py construct_nvmf_subsystem nqn.2016-06.io.spdk:cnode1 "trtype:RDMA traddr:$NVMF_FIRST_TARGET_IP trsvcid:4420" "" -a -s SPDK00000000000001



for bdev in $bdevs; do

	$rpc_py nvmf_subsystem_add_ns nqn.2016-06.io.spdk:cnode1 "$bdev"

done



nvme connect -t rdma -n "nqn.2016-06.io.spdk:cnode1" -a "$NVMF_FIRST_TARGET_IP" -s "$NVMF_PORT"

test/unit/lib/nvmf/subsystem.c/subsystem_ut.c

+7 −2
Original line number Diff line number Diff line
@@ -211,13 +211,15 @@ test_spdk_nvmf_subsystem_add_ns(void) 
	CU_ASSERT(nsid == 1);

	CU_ASSERT(subsystem.max_nsid == 1);

	SPDK_CU_ASSERT_FATAL(subsystem.ns != NULL);

	CU_ASSERT(subsystem.ns[nsid - 1].bdev == &bdev1);

	SPDK_CU_ASSERT_FATAL(subsystem.ns[nsid - 1] != NULL);

	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &bdev1);



	/* Request a specific NSID */

	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev2, 5);

	CU_ASSERT(nsid == 5);

	CU_ASSERT(subsystem.max_nsid == 5);

	CU_ASSERT(subsystem.ns[nsid - 1].bdev == &bdev2);

	SPDK_CU_ASSERT_FATAL(subsystem.ns[nsid - 1] != NULL);

	CU_ASSERT(subsystem.ns[nsid - 1]->bdev == &bdev2);



	/* Request an NSID that is already in use */

	nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev2, 5);
@@ -229,6 +231,9 @@ test_spdk_nvmf_subsystem_add_ns(void) 
	CU_ASSERT(nsid == 0);

	CU_ASSERT(subsystem.max_nsid == 5);



	spdk_nvmf_subsystem_remove_ns(&subsystem, 1);

	spdk_nvmf_subsystem_remove_ns(&subsystem, 5);



	free(subsystem.ns);

}