ARM: TLS pointer may not be set correctly

[andrewboie]

Describe the bug

It's expected that given the following code:

#define SIZE       4096
struct k_thread thread;
K_THREAD_STACK_DEFINE(stack, SIZE);

That it is valid to pass either SIZE or K_THREAD_STACK_SIZEOF(stack) to a k_thread_create() call's stack size argument.

This is not the case for ARM Cortex-M. If SIZE is passed in, the math is performed improperly and the TLS pointer gets set to memory after the end of the stack object. This will result in either MPU faults when trying to access TLS (at the moment, limited to getting/setting errno) or data corruption.

The following test code illustrates the issue. Only ARM Cortex-M seems to be affected.

diff --git a/tests/kernel/mem_protect/userspace/src/main.c b/tests/kernel/mem_protect/userspace/src/mai
index 4eb34e0407..71f8862ddc 100644
--- a/tests/kernel/mem_protect/userspace/src/main.c
+++ b/tests/kernel/mem_protect/userspace/src/main.c
@@ -1131,6 +1131,43 @@ void test_object_recycle(void)
        zassert_true(perms_count == 1, "invalid number of thread permissions");
 }

+#define TLS_SIZE       4096
+struct k_thread tls_thread;
+K_THREAD_STACK_DEFINE(tls_stack, TLS_SIZE);
+
+void tls_entry(void *p1, void *p2, void *p3)
+{
+       printk("tls_entry\n");
+}
+
+void test_tls_pointer(void)
+{
+       k_thread_create(&tls_thread, tls_stack, TLS_SIZE, tls_entry,
+                       NULL, NULL, NULL, 1, K_USER, K_FOREVER);
+
+       printk("tls pointer for thread %p: %p\n",
+              &tls_thread, (void *)tls_thread.userspace_local_data);
+
+       printk("stack buffer reported bounds: [%p, %p)\n",
+              (void *)tls_thread.stack_info.start,
+              (void *)(tls_thread.stack_info.start +
+                       tls_thread.stack_info.size));
+
+       printk("stack object bounds: [%p, %p)\n",
+              tls_stack, tls_stack + sizeof(tls_stack));
+
+       uintptr_t tls_start = (uintptr_t)tls_thread.userspace_local_data;
+       uintptr_t tls_end = tls_start +
+               sizeof(struct _thread_userspace_local_data);
+
+       if ((tls_start < (uintptr_t)tls_stack) ||
+           (tls_end > (uintptr_t)tls_stack + sizeof(tls_stack))) {
+               printk("tls area out of bounds\n");
+               ztest_test_fail();
+       }
+}
+
+
 void test_main(void)
 {
        struct k_mem_partition *parts[] = {&part0, &part1,
@@ -1184,7 +1221,8 @@ void test_main(void)
                         ztest_unit_test(test_stack_buffer),
                         ztest_user_unit_test(test_unimplemented_syscall),
                         ztest_user_unit_test(test_bad_syscall),
-                        ztest_unit_test(test_object_recycle)
+                        ztest_unit_test(test_object_recycle),
+                        ztest_unit_test(test_tls_pointer)
                         );
        ztest_run_test_suite(userspace);
 }

To Reproduce
Run above test code. You will see:

===================================================================
starting test - test_tls_pointer
tls pointer for thread 0x200088a8: 0x2001d018
stack buffer reported bounds: [0x2001c020, 0x2001d000)
stack object bounds: [0x2001c000, 0x2001d000)
tls area out of bounds
FAIL - test_tls_pointer

Expected behavior
Passing test. TLS region should be within bounds of the stack object.

Impact
Workaround is possible by always using K_THREAD_STACK_SIZEOF(), but users who don't know about this could get MPU faults or data corruption.

My PR #24714 fixes this as I rewrote all the relevant code.
For 1.14 branch I need a more targeted solution.

[ioannisg]

@andrewboie thanks for reporting this. A few notes:

It's expected that given the following code:

#define SIZE 4096
struct k_thread thread;
K_THREAD_STACK_DEFINE(stack, SIZE);
That it is valid to pass either SIZE or K_THREAD_STACK_SIZEOF(stack) to a k_thread_create() call's >stack size argument.

If this is the case (I believe we really need to make this clear in the Macro documentation), then the error is in the kernel implementation of k_thread_create:
k_thread_create() passes the stack_size "as-is" to z_setup_new_thread, but z_setup_new_thread expects the available size, not the desired size:

/*
 * Note:
 * The caller must guarantee that the stack_size passed here corresponds
 * to the amount of stack memory available for the thread.
 */
void z_setup_new_thread(struct k_thread *new_thread,

So this needs to be fixed, but should not affect the bug you are reporting here.

If I understood correctly, the real problem with TLS here is that in ARM's arch_new_thread()
this code block:

#if defined(CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT) \
	&& defined(CONFIG_USERSPACE)
	/* This is required to work-around the case where the thread
	 * is created without using K_THREAD_STACK_SIZEOF() macro in
	 * k_thread_create(). If K_THREAD_STACK_SIZEOF() is used, the
	 * Guard size has already been take out of stackSize.
	 */
	stackSize -= MPU_GUARD_ALIGN_AND_SIZE;
#endif

is executed after setting the userspace local data pointer. It should come first, IMHO, and, unfortunately, I cannot recall why this is not the case.

[ioannisg]

@andrewboie, #25654, contains a fix proposal, and your TLS test for mem_protect/userspace, taken from #25636 (had to resolve conflicts while cherry-picking)

[andrewboie]

@ioannisg none of this stuff is particularly consistent across architectures.
This is why I rewrote all of it in #24714. I would really appreciate if you would review that if you have time.