core: do not use virtual addresses as session identifier

Session context virtual address is returned to the REE in
entry_open_session(); it is then used back in entry_close_session() and
entry_invoke_command(). Sharing virtual addresses with the REE leads to
virtual memory addresses disclosure that could be leveraged to defeat
ASLR (if/when implemented) and/or mount an attack.

Similarly, syscall_open_ta_session() returns a session ID directly
derived from the session virtual address to the caller TA.

This commit introduces a 32-bit identifier field in struct tee_ta_session.
The ID is generated when the session is created, starting from the id of
the last session in the queue, and counting up until a number that is not
used in the session queue is found.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [2.1]
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

4 files changed, 66 insertions, 14 deletions

diff --git a/core/arch/arm/tee/entry_std.c b/core/arch/arm/tee/entry_std.c
index e333d774..e02d2917 100644
--- a/core/arch/arm/tee/entry_std.c
+++ b/core/arch/arm/tee/entry_std.c
@@ -330,7 +330,7 @@ cleanup_shm_refs:
 
 out:
 	if (s)
-		arg->session = (vaddr_t)s;
+		arg->session = s->id;
 	else
 		arg->session = 0;
 	arg->ret = res;
@@ -352,7 +352,7 @@ static void entry_close_session(struct thread_smc_args *smc_args,
 	plat_prng_add_jitter_entropy(CRYPTO_RNG_SRC_JITTER_SESSION,
 				     &session_pnum);
 
-	s = (struct tee_ta_session *)(vaddr_t)arg->session;
+	s = tee_ta_find_session(arg->session, &tee_open_sessions);
 	res = tee_ta_close_session(s, &tee_open_sessions, NSAPP_IDENTITY);
 out:
 	arg->ret = res;
diff --git a/core/include/kernel/tee_ta_manager.h b/core/include/kernel/tee_ta_manager.h
index e4e66c8b..255c3d48 100644
--- a/core/include/kernel/tee_ta_manager.h
+++ b/core/include/kernel/tee_ta_manager.h
@@ -90,6 +90,7 @@ struct tee_ta_ctx {
 struct tee_ta_session {
 	TAILQ_ENTRY(tee_ta_session) link;
 	TAILQ_ENTRY(tee_ta_session) link_tsd;
+	uint32_t id;		/* Session handle (0 is invalid) */
 	struct tee_ta_ctx *ctx;	/* TA context */
 	TEE_Identity clnt_id;	/* Identify of client */
 	bool cancel;		/* True if TAF is cancelled */
@@ -149,6 +150,9 @@ struct tee_ta_session *tee_ta_pop_current_session(void);
 
 struct tee_ta_session *tee_ta_get_calling_session(void);
 
+struct tee_ta_session *tee_ta_find_session(uint32_t id,
+			struct tee_ta_session_head *open_sessions);
+
 struct tee_ta_session *tee_ta_get_session(uint32_t id, bool exclusive,
 			struct tee_ta_session_head *open_sessions);
 
diff --git a/core/kernel/tee_ta_manager.c b/core/kernel/tee_ta_manager.c
index 0a6c6ed4..3d4444cc 100644
--- a/core/kernel/tee_ta_manager.c
+++ b/core/kernel/tee_ta_manager.c
@@ -173,16 +173,34 @@ void tee_ta_put_session(struct tee_ta_session *s)
 	mutex_unlock(&tee_ta_mutex);
 }
 
-static struct tee_ta_session *find_session(uint32_t id,
+static struct tee_ta_session *tee_ta_find_session_nolock(uint32_t id,
 			struct tee_ta_session_head *open_sessions)
 {
-	struct tee_ta_session *s;
+	struct tee_ta_session *s = NULL;
+	struct tee_ta_session *found = NULL;
 
 	TAILQ_FOREACH(s, open_sessions, link) {
-		if ((vaddr_t)s == id)
-			return s;
+		if (s->id == id) {
+			found = s;
+			break;
+		}
 	}
-	return NULL;
+
+	return found;
+}
+
+struct tee_ta_session *tee_ta_find_session(uint32_t id,
+			struct tee_ta_session_head *open_sessions)
+{
+	struct tee_ta_session *s = NULL;
+
+	mutex_lock(&tee_ta_mutex);
+
+	s = tee_ta_find_session_nolock(id, open_sessions);
+
+	mutex_unlock(&tee_ta_mutex);
+
+	return s;
 }
 
 struct tee_ta_session *tee_ta_get_session(uint32_t id, bool exclusive,
@@ -193,7 +211,7 @@ struct tee_ta_session *tee_ta_get_session(uint32_t id, bool exclusive,
 	mutex_lock(&tee_ta_mutex);
 
 	while (true) {
-		s = find_session(id, open_sessions);
+		s = tee_ta_find_session_nolock(id, open_sessions);
 		if (!s)
 			break;
 		if (s->unlink) {
@@ -377,12 +395,12 @@ TEE_Result tee_ta_close_session(struct tee_ta_session *csess,
 	struct tee_ta_ctx *ctx;
 	bool keep_alive;
 
-	DMSG("tee_ta_close_session(0x%" PRIxVA ")",  (vaddr_t)csess);
+	DMSG("csess 0x%" PRIxVA " id %u", (vaddr_t)csess, csess->id);
 
 	if (!csess)
 		return TEE_ERROR_ITEM_NOT_FOUND;
 
-	sess = tee_ta_get_session((vaddr_t)csess, true, open_sessions);
+	sess = tee_ta_get_session(csess->id, true, open_sessions);
 
 	if (!sess) {
 		EMSG("session 0x%" PRIxVA " to be removed is not found",
@@ -463,6 +481,31 @@ static TEE_Result tee_ta_init_session_with_context(struct tee_ta_ctx *ctx,
 	return TEE_SUCCESS;
 }
 
+static uint32_t new_session_id(struct tee_ta_session_head *open_sessions)
+{
+	struct tee_ta_session *last = NULL;
+	uint32_t saved = 0;
+	uint32_t id = 1;
+
+	last = TAILQ_LAST(open_sessions, tee_ta_session_head);
+	if (last) {
+		/* This value is less likely to be already used */
+		id = last->id + 1;
+		if (!id)
+			id++; /* 0 is not valid */
+	}
+
+	saved = id;
+	do {
+		if (!tee_ta_find_session_nolock(id, open_sessions))
+			return id;
+		id++;
+		if (!id)
+			id++;
+	} while (id != saved);
+
+	return 0;
+}
 
 static TEE_Result tee_ta_init_session(TEE_ErrorOrigin *err,
 				struct tee_ta_session_head *open_sessions,
@@ -492,6 +535,11 @@ static TEE_Result tee_ta_init_session(TEE_ErrorOrigin *err,
 
 
 	mutex_lock(&tee_ta_mutex);
+	s->id = new_session_id(open_sessions);
+	if (!s->id) {
+		res = TEE_ERROR_OVERFLOW;
+		goto out;
+	}
 	TAILQ_INSERT_TAIL(open_sessions, s, link);
 
 	/* Look for already loaded TA */
diff --git a/core/tee/tee_svc.c b/core/tee/tee_svc.c
index 2be5b3f5..0f01e22a 100644
--- a/core/tee/tee_svc.c
+++ b/core/tee/tee_svc.c
@@ -789,7 +789,7 @@ TEE_Result syscall_open_ta_session(const TEE_UUID *dest,
 function_exit:
 	mobj_free(mobj_param);
 	if (res == TEE_SUCCESS)
-		tee_svc_copy_kaddr_to_uref(ta_sess, s);
+		tee_svc_copy_to_user(ta_sess, &s->id, sizeof(s->id));
 	tee_svc_copy_to_user(ret_orig, &ret_o, sizeof(ret_o));
 
 out_free_only:
@@ -804,13 +804,14 @@ TEE_Result syscall_close_ta_session(unsigned long ta_sess)
 	TEE_Result res;
 	struct tee_ta_session *sess;
 	TEE_Identity clnt_id;
-	struct tee_ta_session *s = tee_svc_uref_to_kaddr(ta_sess);
+	struct tee_ta_session *s = NULL;
 	struct user_ta_ctx *utc;
 
 	res = tee_ta_get_current_session(&sess);
 	if (res != TEE_SUCCESS)
 		return res;
 	utc = to_user_ta_ctx(sess->ctx);
+	s = tee_ta_find_session(ta_sess, &utc->open_sessions);
 
 	clnt_id.login = TEE_LOGIN_TRUSTED_APP;
 	memcpy(&clnt_id.uuid, &sess->ctx->uuid, sizeof(TEE_UUID));
@@ -838,8 +839,7 @@ TEE_Result syscall_invoke_ta_command(unsigned long ta_sess,
 		return res;
 	utc = to_user_ta_ctx(sess->ctx);
 
-	called_sess = tee_ta_get_session(
-				(vaddr_t)tee_svc_uref_to_kaddr(ta_sess), true,
+	called_sess = tee_ta_get_session((uint32_t)ta_sess, true,
 				&utc->open_sessions);
 	if (!called_sess)
 		return TEE_ERROR_BAD_PARAMETERS;