[Qemu-devel] [PATCH] aio: Fix use-after-free in cancellation path

[Fam Zheng]

The current flow of canceling a thread from THREAD_ACTIVE state is:

  1) Caller wants to cancel a request, so it calls thread_pool_cancel.

  2) thread_pool_cancel waits on the conditional variable
     elem->check_cancel.

  3) The worker thread changes state to THREAD_DONE once the task is
     done, and notifies elem->check_cancel to allow thread_pool_cancel
     to continue execution, and signals the notifier (pool->notifier) to
     allow callback function to be called later. But because of the
     global mutex, the notifier won't get processed until step 4) and 5)
     are done.

  4) thread_pool_cancel continues, leaving the notifier signaled, it
     just returns to caller.

  5) Caller thinks the request is already canceled successfully, so it
     releases any related data, such as freeing elem->common.opaque.

  6) In the next main loop iteration, the notifier handler,
     event_notifier_ready, is called. It finds the canceled thread in
     THREAD_DONE state, so calls elem->common.cb, with an (likely)
     dangling opaque pointer. This is a use-after-free.

Fix it by calling elem->common.cb right before thread_pool_cancel
returns.

We leave the notifier signaled to allow any other acbs to be processed.

Reported-by: Ulrich Obergfell <address@hidden>
Suggested-by: Stefan Hajnoczi <address@hidden>
Signed-off-by: Fam Zheng <address@hidden>
---
 thread-pool.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/thread-pool.c b/thread-pool.c
index fbdd3ff..cc1c9f1 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -168,6 +168,24 @@ static void spawn_thread(ThreadPool *pool)
     }
 }
 
+/* Complete one thread and call common.cb if it has returned.
+ * Returns true if commmon.cb is called.
+ */
+static bool thread_pool_complete_one(ThreadPoolElement *elem)
+{
+    bool ret = false;
+
+    QLIST_REMOVE(elem, all);
+    if (elem->state == THREAD_DONE && elem->common.cb) {
+        /* Read state before ret.  */
+        smp_rmb();
+        elem->common.cb(elem->common.opaque, elem->ret);
+        ret = true;
+    }
+    qemu_aio_release(elem);
+    return ret;
+}
+
 static void event_notifier_ready(EventNotifier *notifier)
 {
     ThreadPool *pool = container_of(notifier, ThreadPool, notifier);
@@ -183,23 +201,15 @@ restart:
             trace_thread_pool_complete(pool, elem, elem->common.opaque,
                                        elem->ret);
         }
-        if (elem->state == THREAD_DONE && elem->common.cb) {
-            QLIST_REMOVE(elem, all);
-            /* Read state before ret.  */
-            smp_rmb();
-            elem->common.cb(elem->common.opaque, elem->ret);
-            qemu_aio_release(elem);
+        if (thread_pool_complete_one(elem)) {
             goto restart;
-        } else {
-            /* remove the request */
-            QLIST_REMOVE(elem, all);
-            qemu_aio_release(elem);
         }
     }
 }
 
 static void thread_pool_cancel(BlockDriverAIOCB *acb)
 {
+    ThreadPoolElement *next;
     ThreadPoolElement *elem = (ThreadPoolElement *)acb;
     ThreadPool *pool = elem->pool;
 
@@ -222,6 +232,16 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
             qemu_cond_wait(&pool->check_cancel, &pool->lock);
         }
         pool->pending_cancellations--;
+
+        /* In the case of THREAD_ACTIVE -> THREAD_DONE, we need to call
+         * callback.  Make sure we do it before returning to caller.
+         */
+        QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
+            if (acb == &elem->common) {
+                thread_pool_complete_one(elem);
+                break;
+            }
+        }
     }
     qemu_mutex_unlock(&pool->lock);
 }
-- 
1.9.2