tee: validate TA-supplied pointers in secure-kernel syscalls

bmolodan · claude · bmolodan · commit f05cfb723b8f · 2026-08-03T17:11:56.000+03:00
The copy helpers tee_svc_copy_to_user/tee_svc_copy_from_user and the live
crypto syscalls (utee_hash_update/final, utee_cipher_init, cipher update,
copy_in_attrs) dereferenced or memcpy'd TA-supplied pointer ranges with their
tee_mmu_check_access_rights() guard commented out. A malicious or buggy TA
could pass a NULL or wrapping range and drive an out-of-bounds copy in the
secure world.

This port has no per-TA MMU/MPU context or region table, and every call site
requests TEE_MEMORY_ACCESS_ANY_OWNER, so ownership cannot (and is not meant to)
be enforced. Implement tee_mmu_check_access_rights() as the accessibility check
that is meaningful here - reject NULL+len and address-space-wrapping ranges,
mirroring cmse_check_address_range()'s end-of-range test on the non-secure
boundary - and restore the guard at every live call site, including the
copy_in_attrs path that stores unvalidated attribute buffers later memcpy'd by
op_attr_secret_value_from_user.

Add a temporary CONFIG_APPS_ACCESS_RIGHTS_TEST negative test (mps2 AN505) that
drives the guard with malformed ranges and asserts TEE_ERROR_ACCESS_DENIED.

Co-Authored-By: Claude Fable 5 &lt;noreply@anthropic.com&gt;
diff --git a/apps/Kconfig b/apps/Kconfig
@@ -34,6 +34,16 @@ config APPS_TEST
 	---help---
 		"GP TEE Client API and TEE Internal API test suite."
 
+config APPS_ACCESS_RIGHTS_TEST
+	bool "TA->secure-kernel pointer-access guard test (temporary)"
+	default n
+	---help---
+		"Temporary negative test that drives tee_mmu_check_access_rights()
+		with malformed TA-supplied pointer ranges (NULL+len, wrapping range)
+		and checks that they are rejected with TEE_ERROR_ACCESS_DENIED while a
+		valid buffer is accepted. Remove once the access-rights fix is
+		verified."
+
 comment "H/W Security exception:"
 
 config APPS_HW_SECURITY_EXCEPTION_EXAMPLE
diff --git a/apps/access_rights_test/access_rights_test.c b/apps/access_rights_test/access_rights_test.c
@@ -0,0 +1,73 @@
+/*
+ * Temporary negative test for the TA->secure-kernel pointer-access guard.
+ *
+ * tee_svc_copy_to_user/copy_from_user and the crypto syscalls
+ * (utee_hash_update/final, utee_cipher_init, cipher update, copy_in_attrs)
+ * validate TA-supplied pointer ranges through tee_mmu_check_access_rights()
+ * before dereferencing them. This test drives that guard directly with
+ * malformed ranges and asserts they are rejected with TEE_ERROR_ACCESS_DENIED,
+ * and that a normal in-range buffer is still accepted.
+ *
+ * The guard is exercised directly rather than through a syscall such as
+ * utee_hash_update(). At boot there is no active TA session, and in this port
+ * tee_ta_get_current_session() does not fail in that case - it returns
+ * TEE_SUCCESS with a NULL current_session - so a session-dependent syscall
+ * would fault on the NULL session rather than reach the range check. Calling
+ * the guard directly tests exactly the logic under test with no such
+ * dependency.
+ *
+ * This app is not part of the product; remove it once the fix is verified.
+ * Gated behind CONFIG_APPS_ACCESS_RIGHTS_TEST.
+ */
+
+#include <stdint.h>
+#include <tee_api_types.h>
+#include <tee/tee_svc.h>
+#include "printf.h"
+
+static int check(const char *name, TEE_Result got, TEE_Result expected)
+{
+	if (got == expected) {
+		printf("  [PASS] %s -> 0x%08x\n", name, (unsigned int)got);
+		return 0;
+	}
+	printf("  [FAIL] %s -> 0x%08x (expected 0x%08x)\n", name,
+	       (unsigned int)got, (unsigned int)expected);
+	return 1;
+}
+
+void tee_access_rights_test(void)
+{
+	int fails = 0;
+	uint32_t rd = TEE_MEMORY_ACCESS_READ | TEE_MEMORY_ACCESS_ANY_OWNER;
+	uint32_t wr = TEE_MEMORY_ACCESS_WRITE | TEE_MEMORY_ACCESS_ANY_OWNER;
+	volatile uint32_t valid = 0;
+
+	printf("== access-rights guard negative test ==\n");
+
+	/* NULL buffer with non-zero length must be rejected. */
+	fails += check("null+len (read)",
+		       tee_mmu_check_access_rights(NULL, rd, 0, 8),
+		       TEE_ERROR_ACCESS_DENIED);
+
+	/* A range that wraps the address space must be rejected. */
+	fails += check("wrapping range (write)",
+		       tee_mmu_check_access_rights(NULL, wr, (uaddr_t)~0UL, 2),
+		       TEE_ERROR_ACCESS_DENIED);
+
+	/* Zero-length range is trivially accepted. */
+	fails += check("zero length",
+		       tee_mmu_check_access_rights(NULL, rd, 0, 0),
+		       TEE_SUCCESS);
+
+	/* A normal in-range buffer is still accepted. */
+	fails += check("valid buffer",
+		       tee_mmu_check_access_rights(NULL, wr, (uaddr_t)&valid,
+						   sizeof(valid)),
+		       TEE_SUCCESS);
+
+	if (fails == 0)
+		printf("== RESULT: all cases handled correctly ==\n");
+	else
+		printf("== RESULT: %d case(s) FAILED ==\n", fails);
+}
diff --git a/arch/cortex-m33/mps2/src/mps2_an505_qemu/secure/Make.defs b/arch/cortex-m33/mps2/src/mps2_an505_qemu/secure/Make.defs
@@ -68,3 +68,7 @@ endif
 ifeq ($(CONFIG_APPS_TEST),y)
 CHIP_CSRCS_S += ../../../../../../apps/test/ta/test_ta.c
 endif
+
+ifeq ($(CONFIG_APPS_ACCESS_RIGHTS_TEST),y)
+CHIP_CSRCS_S += ../../../../../../apps/access_rights_test/access_rights_test.c
+endif
diff --git a/arch/cortex-m33/mps2/src/mps2_an505_qemu/secure/main.c b/arch/cortex-m33/mps2/src/mps2_an505_qemu/secure/main.c
@@ -81,6 +81,10 @@ void menu_security_exception_example(void);
 
 extern TEE_Result tee_cryp_init(void);
 
+#ifdef CONFIG_APPS_ACCESS_RIGHTS_TEST
+extern void tee_access_rights_test(void);
+#endif
+
 ///* Private Functions. */
 //
 ///**
@@ -685,6 +689,10 @@ int main(void)
 #endif
 
  tee_cryp_init();
+
+#ifdef CONFIG_APPS_ACCESS_RIGHTS_TEST
+  tee_access_rights_test();
+#endif
 //
 //  Boot_Init(CONFIG_START_ADDRESS_BL33);
 
diff --git a/tee/include/tee/tee_svc.h b/tee/include/tee/tee_svc.h
@@ -88,6 +88,11 @@ TEE_Result syscall_invoke_ta_command(unsigned long sess,
 TEE_Result syscall_check_access_rights(unsigned long flags, const void *buf,
 				       size_t len);
 
+struct user_ta_ctx;
+
+TEE_Result tee_mmu_check_access_rights(struct user_ta_ctx *utc, uint32_t flags,
+				       uaddr_t uaddr, size_t len);
+
 TEE_Result tee_svc_copy_from_user(void *kaddr, const void *uaddr, size_t len);
 TEE_Result tee_svc_copy_to_user(void *uaddr, const void *kaddr, size_t len);
 
diff --git a/tee/tee/tee_svc.c b/tee/tee/tee_svc.c
@@ -926,41 +926,74 @@ vaddr_t tee_svc_uref_base = 0;
 //					   (uaddr_t)buf, len);
 //}
 //
+/*
+ * tee_mmu_check_access_rights - validate a TA-supplied pointer range.
+ *
+ * In this ARMv8-M port TAs are statically linked into the secure image and run
+ * privileged in a single flat address space: there is no per-TA MMU/MPU context
+ * or region table (utc->mmu is not populated), so the OP-TEE-style ownership and
+ * secure/non-secure attribution this signature was designed for have no backing
+ * state. Every live caller requests TEE_MEMORY_ACCESS_ANY_OWNER, i.e. "do not
+ * enforce per-TA ownership, just accessibility", so this validates the property
+ * that is actually meaningful here: the range is a real, non-wrapping range. This
+ * mirrors the wrap/overflow guard cmse_check_address_range() performs on the
+ * non-secure boundary and rejects the dangerous primitive of an attacker- or
+ * bug-supplied length that would drive an out-of-bounds copy.
+ */
+TEE_Result tee_mmu_check_access_rights(struct user_ta_ctx *utc, uint32_t flags,
+				       uaddr_t uaddr, size_t len)
+{
+	(void)utc;	/* unused: no per-TA region data here; callers pass NULL */
+	(void)flags;	/* ANY_OWNER: ownership is not (and can not be) enforced */
+
+	if (!len)
+		return TEE_SUCCESS;		/* zero-length range is trivially OK */
+	if (!uaddr)
+		return TEE_ERROR_ACCESS_DENIED;
+	/*
+	 * Reject a range that wraps the address space. len is non-zero here, so
+	 * the last byte is at uaddr + len - 1; if that is below uaddr the range
+	 * overflowed. Matches cmse_check_address_range()'s end-of-range test and
+	 * still accepts a range whose last byte sits at the top of memory.
+	 */
+	if (uaddr + len - 1 < uaddr)
+		return TEE_ERROR_ACCESS_DENIED;
+
+	return TEE_SUCCESS;
+}
+
 TEE_Result tee_svc_copy_from_user(void *kaddr, const void *uaddr, size_t len)
 {
-//	TEE_Result res;
-//	struct tee_ta_session *s;
-//
-//	res = tee_ta_get_current_session(&s);
-//	if (res != TEE_SUCCESS)
-//		return res;
-//
-//	res = tee_mmu_check_access_rights(to_user_ta_ctx(s->ctx),
-//					TEE_MEMORY_ACCESS_READ |
-//					TEE_MEMORY_ACCESS_ANY_OWNER,
-//					(uaddr_t)uaddr, len);
-//	if (res != TEE_SUCCESS)
-//		return res;
-//
+	TEE_Result res;
+
+	/*
+	 * No session lookup here: tee_mmu_check_access_rights() ignores the utc
+	 * in this port, and tee_ta_get_current_session() returns TEE_SUCCESS with
+	 * a NULL current_session when no TA session is active, so dereferencing
+	 * it (to_user_ta_ctx(s->ctx)) would fault instead of failing gracefully.
+	 */
+	res = tee_mmu_check_access_rights(NULL,
+					TEE_MEMORY_ACCESS_READ |
+					TEE_MEMORY_ACCESS_ANY_OWNER,
+					(uaddr_t)uaddr, len);
+	if (res != TEE_SUCCESS)
+		return res;
+
 	memcpy(kaddr, uaddr, len);
 	return TEE_SUCCESS;
 }
 
 TEE_Result tee_svc_copy_to_user(void *uaddr, const void *kaddr, size_t len)
 {
-//	TEE_Result res;
-//	struct tee_ta_session *s;
-//
-//	res = tee_ta_get_current_session(&s);
-//	if (res != TEE_SUCCESS)
-//		return res;
-//
-//	res = tee_mmu_check_access_rights(to_user_ta_ctx(s->ctx),
-//					TEE_MEMORY_ACCESS_WRITE |
-//					TEE_MEMORY_ACCESS_ANY_OWNER,
-//					(uaddr_t)uaddr, len);
-//	if (res != TEE_SUCCESS)
-//		return res;
+	TEE_Result res;
+
+	/* See tee_svc_copy_from_user() on why no session is looked up here. */
+	res = tee_mmu_check_access_rights(NULL,
+					TEE_MEMORY_ACCESS_WRITE |
+					TEE_MEMORY_ACCESS_ANY_OWNER,
+					(uaddr_t)uaddr, len);
+	if (res != TEE_SUCCESS)
+		return res;
 
 	memcpy(uaddr, kaddr, len);
 	return TEE_SUCCESS;
diff --git a/tee/tee/tee_svc_cryp.c b/tee/tee/tee_svc_cryp.c
@@ -1337,15 +1337,19 @@ static TEE_Result copy_in_attrs(struct user_ta_ctx *utc,
 			const struct utee_attribute *usr_attrs,
 			uint32_t attr_count, TEE_Attribute *attrs)
 {
-//	TEE_Result res;
+	TEE_Result res;
 	uint32_t n;
-  (void) utc;
-//	res = tee_mmu_check_access_rights(utc,
-//			TEE_MEMORY_ACCESS_READ | TEE_MEMORY_ACCESS_ANY_OWNER,
-//			(uaddr_t)usr_attrs,
-//			attr_count * sizeof(struct utee_attribute));
-//	if (res != TEE_SUCCESS)
-//		return res;
+	size_t attrs_size;
+
+	/* Guard the range length against a 32-bit multiply wrap. */
+	if (MUL_OVERFLOW(attr_count, sizeof(struct utee_attribute), &attrs_size))
+		return TEE_ERROR_OVERFLOW;
+
+	res = tee_mmu_check_access_rights(utc,
+			TEE_MEMORY_ACCESS_READ | TEE_MEMORY_ACCESS_ANY_OWNER,
+			(uaddr_t)usr_attrs, attrs_size);
+	if (res != TEE_SUCCESS)
+		return res;
 
 	for (n = 0; n < attr_count; n++) {
 		attrs[n].attributeID = usr_attrs[n].attribute_id;
@@ -1356,11 +1360,11 @@ static TEE_Result copy_in_attrs(struct user_ta_ctx *utc,
 			uintptr_t buf = usr_attrs[n].a;
 			size_t len = usr_attrs[n].b;
 
-//			res = tee_mmu_check_access_rights(utc,
-//				TEE_MEMORY_ACCESS_READ |
-//				TEE_MEMORY_ACCESS_ANY_OWNER, buf, len);
-//			if (res != TEE_SUCCESS)
-//				return res;
+			res = tee_mmu_check_access_rights(utc,
+				TEE_MEMORY_ACCESS_READ |
+				TEE_MEMORY_ACCESS_ANY_OWNER, buf, len);
+			if (res != TEE_SUCCESS)
+				return res;
 			attrs[n].content.ref.buffer = (void *)buf;
 			attrs[n].content.ref.length = len;
 		}
@@ -2231,12 +2235,12 @@ TEE_Result utee_hash_update(unsigned long state, const void *chunk,
 	if (res != TEE_SUCCESS)
 		return res;
 
-//	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
-//					  TEE_MEMORY_ACCESS_READ |
-//					  TEE_MEMORY_ACCESS_ANY_OWNER,
-//					  (uaddr_t)chunk, chunk_size);
-//	if (res != TEE_SUCCESS)
-//		return res;
+	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
+					  TEE_MEMORY_ACCESS_READ |
+					  TEE_MEMORY_ACCESS_ANY_OWNER,
+					  (uaddr_t)chunk, chunk_size);
+	if (res != TEE_SUCCESS)
+		return res;
 
 	res = tee_svc_cryp_get_state(sess, tee_svc_uref_to_vaddr(state), &cs);
 	if (res != TEE_SUCCESS)
@@ -2277,24 +2281,24 @@ TEE_Result utee_hash_final(unsigned long state, const void *chunk,
 	if (res != TEE_SUCCESS)
 		return res;
 
-//	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
-//					  TEE_MEMORY_ACCESS_READ |
-//					  TEE_MEMORY_ACCESS_ANY_OWNER,
-//					  (uaddr_t)chunk, chunk_size);
-//	if (res != TEE_SUCCESS)
-//		return res;
+	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
+					  TEE_MEMORY_ACCESS_READ |
+					  TEE_MEMORY_ACCESS_ANY_OWNER,
+					  (uaddr_t)chunk, chunk_size);
+	if (res != TEE_SUCCESS)
+		return res;
 
 	res = tee_svc_copy_from_user(&hlen, hash_len, sizeof(hlen));
 	if (res != TEE_SUCCESS)
 		return res;
 
-//	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
-//					  TEE_MEMORY_ACCESS_READ |
-//					  TEE_MEMORY_ACCESS_WRITE |
-//					  TEE_MEMORY_ACCESS_ANY_OWNER,
-//					  (uaddr_t)hash, hlen);
-//	if (res != TEE_SUCCESS)
-//		return res;
+	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
+					  TEE_MEMORY_ACCESS_READ |
+					  TEE_MEMORY_ACCESS_WRITE |
+					  TEE_MEMORY_ACCESS_ANY_OWNER,
+					  (uaddr_t)hash, hlen);
+	if (res != TEE_SUCCESS)
+		return res;
 
 	res = tee_svc_cryp_get_state(sess, tee_svc_uref_to_vaddr(state), &cs);
 	if (res != TEE_SUCCESS)
@@ -2373,13 +2377,13 @@ TEE_Result utee_cipher_init(unsigned long state, const void *iv,
 	if (res != TEE_SUCCESS)
 		return res;
 
-//	res = tee_mmu_check_access_rights(utc,
-//					  TEE_MEMORY_ACCESS_READ |
-//					  TEE_MEMORY_ACCESS_ANY_OWNER,
-//					  (uaddr_t) iv, iv_len);
-//	if (res != TEE_SUCCESS)
-//		return res;
-//
+	res = tee_mmu_check_access_rights(utc,
+					  TEE_MEMORY_ACCESS_READ |
+					  TEE_MEMORY_ACCESS_ANY_OWNER,
+					  (uaddr_t) iv, iv_len);
+	if (res != TEE_SUCCESS)
+		return res;
+
 	res = tee_obj_get(utc, cs->key1, &o);
 	if (res != TEE_SUCCESS)
 		return res;
@@ -2427,12 +2431,12 @@ static TEE_Result tee_svc_cipher_update_helper(unsigned long state,
 	if (res != TEE_SUCCESS)
 		return res;
 
-//	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
-//					  TEE_MEMORY_ACCESS_READ |
-//					  TEE_MEMORY_ACCESS_ANY_OWNER,
-//					  (uaddr_t)src, src_len);
-//	if (res != TEE_SUCCESS)
-//		return res;
+	res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
+					  TEE_MEMORY_ACCESS_READ |
+					  TEE_MEMORY_ACCESS_ANY_OWNER,
+					  (uaddr_t)src, src_len);
+	if (res != TEE_SUCCESS)
+		return res;
 
 	if (!dst_len) {
 		dlen = 0;
@@ -2441,13 +2445,13 @@ static TEE_Result tee_svc_cipher_update_helper(unsigned long state,
 		if (res != TEE_SUCCESS)
 			return res;
 
-//		res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
-//						  TEE_MEMORY_ACCESS_READ |
-//						  TEE_MEMORY_ACCESS_WRITE |
-//						  TEE_MEMORY_ACCESS_ANY_OWNER,
-//						  (uaddr_t)dst, dlen);
-//		if (res != TEE_SUCCESS)
-//			return res;
+		res = tee_mmu_check_access_rights(to_user_ta_ctx(sess->ctx),
+						  TEE_MEMORY_ACCESS_READ |
+						  TEE_MEMORY_ACCESS_WRITE |
+						  TEE_MEMORY_ACCESS_ANY_OWNER,
+						  (uaddr_t)dst, dlen);
+		if (res != TEE_SUCCESS)
+			return res;
 	}
 
 	if (dlen < src_len) {
