From: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Date: Thu, 27 Jan 2022 10:07:58 +0000 (+0100)
Subject: Fix use-after-free in glue code for pthread_join
X-Git-Tag: RELEASE-0.8.0^0
X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=e2705f98bcb17f423547c2394cc672a52de3d1e4;p=unikraft%2Flibs%2Fpthread-embedded.git

Fix use-after-free in glue code for pthread_join

When pthread_join is called, we use uk_thread_wait to wait for the
thread to exit. However, this will also release the thread and the
metadata for the pthread in the glue code. pthread_join then calls
pthread_detach, which attempts another wait, accessing the freed
thread.

The commit changes the pte_osThreadHandle to point to the
metadata in the glue code instead of the thread itself and prevents
the metadata from being released on exit of the uk thread. This way,
pthread_detach can detect that the thread has already been released.
Metadata is freed in pte_osThreadDelete (would have caused a double
free before the patch).

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Reviewed-by: Vlad-Andrei <vlad_andrei.badoiu@upb.ro>
Approved-by: Simon Kuenzer <simon@unikraft.org>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Pull-Request: #2
---

diff --git a/include/pte_osal.h b/include/pte_osal.h
index 759cb1b..b837b3e 100644
--- a/include/pte_osal.h
+++ b/include/pte_osal.h
@@ -8,7 +8,7 @@
 extern "C" {
 #endif
 
-typedef struct uk_thread* pte_osThreadHandle;
+typedef struct pte_thread_data *pte_osThreadHandle;
 typedef struct uk_semaphore *pte_osSemaphoreHandle;
 typedef struct uk_mutex *pte_osMutexHandle;
 
diff --git a/pte_osal.c b/pte_osal.c
index d23e6b3..27c4baa 100644
--- a/pte_osal.c
+++ b/pte_osal.c
@@ -37,7 +37,7 @@
 #include "tls-helper.h"
 
 
-typedef struct {
+typedef struct pte_thread_data {
 	/* thread routine */
 	pte_osThreadEntryPoint entry_point;
 	/* thread routine arguments */
@@ -120,7 +120,7 @@ out:
 #if CONFIG_LIBUKSIGNAL
 int pte_kill(pte_osThreadHandle threadId, int sig)
 {
-	return uk_sig_thread_kill(threadId, sig);
+	return uk_sig_thread_kill(threadId->uk_thread, sig);
 }
 #endif
 
@@ -133,7 +133,7 @@ int pte_kill(pte_osThreadHandle threadId, int sig)
 
 static pte_thread_data_t *handle_to_ptd(pte_osThreadHandle h)
 {
-	return h->prv;
+	return h;
 }
 
 static pte_thread_data_t *current_ptd(void)
@@ -196,7 +196,7 @@ pte_osResult pte_osThreadCreate(pte_osThreadEntryPoint entry_point,
 	UK_ASSERT(th->prv != NULL);
 
 	/* Return the thread handle */
-	*ph = th;
+	*ph = th->prv;
 	return PTE_OS_OK;
 }
 
@@ -279,19 +279,7 @@ err_out:
 
 static void pte_osFiniThread(struct uk_thread *th)
 {
-	pte_thread_data_t *ptd;
-
-	if (!th->prv) {
-		/* It seems that this is a thread that was created before
-		 * this library was initialized. We should not have
-		 * allocated anything for this thread.
-		 */
-		return;
-	}
-
-	ptd = th->prv;
-	pteTlsThreadDestroy(ptd->tls);
-	free(ptd);
+	/* We clean up resources in pte_osThreadDelete() */
 }
 
 UK_THREAD_INIT_PRIO(pte_osInitThread, pte_osFiniThread, UK_PRIO_EARLIEST);
@@ -319,8 +307,11 @@ pte_osResult pte_osThreadDelete(pte_osThreadHandle h)
 
 pte_osResult pte_osThreadExitAndDelete(pte_osThreadHandle h)
 {
-	if (h->sched)
-		uk_thread_kill(h);
+	pte_thread_data_t *ptd = handle_to_ptd(h);
+	UK_ASSERT(ptd->uk_thread);
+
+	if (ptd->uk_thread->sched)
+		uk_thread_kill(ptd->uk_thread);
 	pte_osThreadDelete(h);
 
 	return PTE_OS_OK;
@@ -341,7 +332,13 @@ pte_osResult pte_osThreadWaitForEnd(pte_osThreadHandle h)
 
 	while (1) {
 		if (ptd->done) {
-			uk_thread_wait(ptd->uk_thread);
+			if (ptd->uk_thread) {
+				uk_thread_wait(ptd->uk_thread);
+
+				/* The thread is destroyed after the wait */
+				ptd->uk_thread = NULL;
+			}
+
 			return PTE_OS_OK;
 		}
 
@@ -374,7 +371,7 @@ pte_osResult pte_osThreadCheckCancel(pte_osThreadHandle h)
 
 pte_osThreadHandle pte_osThreadGetHandle(void)
 {
-	return uk_thread_current();
+	return current_ptd();
 }
 
 int pte_osThreadGetPriority(pte_osThreadHandle h)