Commit 33af6579 authored by Jinlong Chen's avatar Jinlong Chen Committed by Tomasz Zawadzki
Browse files

lib/thread: fix use-after-free in spdk_for_each_channel 



In the following sequence there will be a use-after-free problem in
spdk_for_each_channel:

1. I/O channel of device D on thread T is obtained and assigned to i->ch
   in spdk_for_each_channel or spdk_for_each_channel_continue.
2. The I/O channel is destroyed.
3. A new I/O channel of device D on thread T is created.
4. _call_channel is called on thread T, i->fn is called because there is
   an I/O channel of device D on thread T. However, the I/O channel that
   i->ch points to has been freed.

To fix this, we assign i->ch in _call_channel instead of
spdk_for_each_channel and spdk_for_each_channel_continue, so that we can
always get the correct I/O channel.

Change-Id: I6d43a3e3842874327d2ac02085c1571283bd787d
Signed-off-by: default avatarJinlong Chen <chenjinlong.cjl@alibaba-inc.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26231


Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
parent 49227970

lib/thread/thread.c

+2 −5
Original line number Diff line number Diff line
@@ -2645,7 +2645,6 @@ static void 
_call_channel(void *ctx)

{

	struct spdk_io_channel_iter *i = ctx;

	struct spdk_io_channel *ch;



	/*

	 * It is possible that the channel was deleted before this
@@ -2653,10 +2652,10 @@ _call_channel(void *ctx) 
	 *  the fn() on this thread.

	 */

	pthread_mutex_lock(&g_devlist_mutex);

	ch = thread_get_io_channel(i->cur_thread, i->dev);

	i->ch = thread_get_io_channel(i->cur_thread, i->dev);

	pthread_mutex_unlock(&g_devlist_mutex);



	if (ch) {

	if (i->ch) {

		i->fn(i);

	} else {

		spdk_for_each_channel_continue(i, 0);
@@ -2708,7 +2707,6 @@ spdk_for_each_channel(void *io_device, spdk_channel_msg fn, void *ctx, 
	if (thr_link != NULL) {

		i->dev->for_each_count++;

		i->cur_thread = thr_link->thread;

		i->ch = thread_get_io_channel(thr_link->thread, i->dev); /* must exist, thread is in tail queue */

		rc = spdk_thread_send_msg(i->cur_thread, _call_channel, i);

		if (rc == 0) {

			pthread_mutex_unlock(&g_devlist_mutex);
@@ -2766,7 +2764,6 @@ spdk_for_each_channel_continue(struct spdk_io_channel_iter *i, int status) 
	thread = io_dev_get_next_thread(i->dev, i->cur_thread);

	if (thread != NULL) {

		i->cur_thread = thread;

		i->ch = thread_get_io_channel(thread, dev);

		rc = spdk_thread_send_msg(i->cur_thread, _call_channel, i);

		if (rc == 0) {

			pthread_mutex_unlock(&g_devlist_mutex);

test/unit/lib/thread/thread.c/thread_ut.c

+58 −1
Original line number Diff line number Diff line
@@ -505,6 +505,12 @@ static void 
channel_msg(struct spdk_io_channel_iter *i)

{

	int *msg_count = spdk_io_channel_iter_get_ctx(i);

	int *ch_ctx = spdk_io_channel_get_ctx(spdk_io_channel_iter_get_channel(i));



	/**

	 * Touch ch_ctx to trigger use-after-free if it was freed.

	 */

	*ch_ctx = 0;



	(*msg_count)++;

	spdk_for_each_channel_continue(i, 0);
@@ -518,12 +524,19 @@ channel_cpl(struct spdk_io_channel_iter *i, int status) 
	(*msg_count)++;

}



static void

_get_channel(void *ctx)

{

	(void)spdk_get_io_channel(ctx);

}



static void

for_each_channel_remove(void)

{

	struct spdk_io_channel *ch0, *ch1, *ch2;

	struct spdk_io_channel *ch0, *ch1, *ch2, *new_ch0;

	int ch_count = 0;

	int msg_count = 0;

	int rc;



	allocate_threads(3);

	set_thread(0);
@@ -569,6 +582,50 @@ for_each_channel_remove(void) 
	poll_threads();

	CU_ASSERT(ch_count == 2);

	CU_ASSERT(msg_count == 4);



	msg_count = 0;



	/*

	 * Case #3: Put the I/O channel, call spdk_for_each_channel, and then poll threads.

	 */

	ch0 = spdk_get_io_channel(&ch_count);

	CU_ASSERT(ch_count == 3);

	spdk_put_io_channel(ch0);

	CU_ASSERT(ch_count == 3);

	spdk_for_each_channel(&ch_count, channel_msg, &msg_count, channel_cpl);

	poll_threads();

	CU_ASSERT(ch_count == 2);

	CU_ASSERT(msg_count == 3);



	msg_count = 0;



	/*

	 * Case #4: Recreate the I/O channel right before channel_msg is called.

	 */

	ch0 = spdk_get_io_channel(&ch_count);

	CU_ASSERT(ch_count == 3);

	spdk_put_io_channel(ch0);

	CU_ASSERT(ch_count == 3);

	rc = spdk_thread_send_msg(spdk_get_thread(), _get_channel, &ch_count);

	CU_ASSERT(rc == 0);

	CU_ASSERT(ch_count == 3);

	spdk_for_each_channel(&ch_count, channel_msg, &msg_count, channel_cpl);

	poll_threads();

	new_ch0 = spdk_get_io_channel(&ch_count);

	CU_ASSERT(ch_count == 3);

	CU_ASSERT(msg_count == 4);



	/**

	 * Put new_ch0 twice to also release the ref got in _get_channel.

	 */

	spdk_put_io_channel(new_ch0);

	spdk_put_io_channel(new_ch0);

	poll_threads();

	CU_ASSERT(ch_count == 2);



	/*

	 * Cleanups.

	 */

	set_thread(1);

	spdk_put_io_channel(ch1);

	CU_ASSERT(ch_count == 2);