core: make stack trace robust

Makes stack trace robust by checking addresses before copying data.
Kernel stack traces are a bit more relaxed as we have crashed already.

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960 AArch32, Aarch64)
Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (Juno, QEMU)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

5 files changed, 175 insertions, 70 deletions

diff --git a/core/arch/arm/include/kernel/unwind.h b/core/arch/arm/include/kernel/unwind.h
index 42176292..7d8f5957 100644
--- a/core/arch/arm/include/kernel/unwind.h
+++ b/core/arch/arm/include/kernel/unwind.h
@@ -42,7 +42,7 @@
 struct unwind_state_arm32 {
 	uint32_t registers[16];
 	uint32_t start_pc;
-	uint32_t *insn;
+	vaddr_t insn;
 	unsigned entries;
 	unsigned byte;
 	uint16_t update_mask;
@@ -52,9 +52,11 @@ struct unwind_state_arm32 {
  * Unwind a 32-bit user or kernel stack.
  * @exidx, @exidx_sz: address and size of the binary search index table
  * (.ARM.exidx section).
+ * @stack, @stack_size: the bottom of the stack and its size, respectively.
  */
-bool unwind_stack_arm32(struct unwind_state_arm32 *state, uaddr_t exidx,
-			size_t exidx_sz);
+bool unwind_stack_arm32(struct unwind_state_arm32 *state, vaddr_t exidx,
+			size_t exidx_sz, bool kernel_stack, vaddr_t stack,
+			size_t stack_size);
 
 #ifdef ARM64
 /* The state of the unwind process (64-bit mode) */
@@ -68,18 +70,19 @@ struct unwind_state_arm64 {
  * Unwind a 64-bit user or kernel stack.
  * @stack, @stack_size: the bottom of the stack and its size, respectively.
  */
-bool unwind_stack_arm64(struct unwind_state_arm64 *state, uaddr_t stack,
-			size_t stack_size);
+bool unwind_stack_arm64(struct unwind_state_arm64 *state, bool kernel_stack,
+			vaddr_t stack, size_t stack_size);
 #endif /*ARM64*/
 
 #if defined(CFG_UNWIND) && (TRACE_LEVEL > 0)
 
 #ifdef ARM64
-void print_stack_arm64(int level, struct unwind_state_arm64 *state, uaddr_t exidx,
-		       size_t exidx_sz);
+void print_stack_arm64(int level, struct unwind_state_arm64 *state,
+		       bool kernel_stack, vaddr_t stack, size_t stack_size);
 #endif
-void print_stack_arm32(int level, struct unwind_state_arm32 *state, uaddr_t exidx,
-		       size_t exidx_sz);
+void print_stack_arm32(int level, struct unwind_state_arm32 *state,
+		       vaddr_t exidx, size_t exidx_sz, bool kernel_stack,
+		       vaddr_t stack, size_t stack_size);
 void print_kernel_stack(int level);
 
 #else /* defined(CFG_UNWIND) && (TRACE_LEVEL > 0) */
diff --git a/core/arch/arm/kernel/abort.c b/core/arch/arm/kernel/abort.c
index e420aa13..a01e5ad1 100644
--- a/core/arch/arm/kernel/abort.c
+++ b/core/arch/arm/kernel/abort.c
@@ -28,7 +28,8 @@ enum fault_type {
 
 #ifdef CFG_UNWIND
 
-static void get_current_ta_exidx(uaddr_t *exidx, size_t *exidx_sz)
+static void get_current_ta_exidx_stack(vaddr_t *exidx, size_t *exidx_sz,
+				       vaddr_t *stack, size_t *stack_size)
 {
 	struct tee_ta_session *s;
 	struct user_ta_ctx *utc;
@@ -45,6 +46,9 @@ static void get_current_ta_exidx(uaddr_t *exidx, size_t *exidx_sz)
 	if (*exidx)
 		*exidx += utc->load_addr;
 	*exidx_sz = utc->exidx_size;
+
+	*stack = utc->stack_addr;
+	*stack_size = utc->mobj_stack->size;
 }
 
 #ifdef ARM32
@@ -55,21 +59,30 @@ static void get_current_ta_exidx(uaddr_t *exidx, size_t *exidx_sz)
 static void __print_stack_unwind_arm32(struct abort_info *ai)
 {
 	struct unwind_state_arm32 state;
-	uaddr_t exidx;
+	vaddr_t exidx;
 	size_t exidx_sz;
 	uint32_t mode = ai->regs->spsr & CPSR_MODE_MASK;
 	uint32_t sp;
 	uint32_t lr;
+	vaddr_t stack;
+	size_t stack_size;
+	bool kernel_stack;
 
 	if (abort_is_user_exception(ai)) {
-		get_current_ta_exidx(&exidx, &exidx_sz);
+		get_current_ta_exidx_stack(&exidx, &exidx_sz, &stack,
+					   &stack_size);
 		if (!exidx) {
 			EMSG_RAW("Call stack not available");
 			return;
 		}
+		kernel_stack = false;
 	} else {
 		exidx = (vaddr_t)__exidx_start;
 		exidx_sz = (vaddr_t)__exidx_end - (vaddr_t)__exidx_start;
+		/* Kernel stack */
+		stack = thread_stack_start();
+		stack_size = thread_stack_size();
+		kernel_stack = true;
 	}
 
 	if (mode == CPSR_MODE_USR || mode == CPSR_MODE_SYS) {
@@ -97,20 +110,23 @@ static void __print_stack_unwind_arm32(struct abort_info *ai)
 	state.registers[14] = lr;
 	state.registers[15] = ai->pc;
 
-	print_stack_arm32(TRACE_ERROR, &state, exidx, exidx_sz);
+	print_stack_arm32(TRACE_ERROR, &state, exidx, exidx_sz, kernel_stack,
+			  stack, stack_size);
 }
 #else /* ARM32 */
 
 static void __print_stack_unwind_arm32(struct abort_info *ai __unused)
 {
 	struct unwind_state_arm32 state;
-	uaddr_t exidx;
+	vaddr_t exidx;
 	size_t exidx_sz;
+	vaddr_t stack;
+	size_t stack_size;
 
 	/* 64-bit kernel, hence 32-bit unwind must be for user mode */
 	assert(abort_is_user_exception(ai));
 
-	get_current_ta_exidx(&exidx, &exidx_sz);
+	get_current_ta_exidx_stack(&exidx, &exidx_sz, &stack, &stack_size);
 
 	memset(&state, 0, sizeof(state));
 	state.registers[0] = ai->regs->x0;
@@ -130,7 +146,8 @@ static void __print_stack_unwind_arm32(struct abort_info *ai __unused)
 	state.registers[14] = ai->regs->x14;
 	state.registers[15] = ai->pc;
 
-	print_stack_arm32(TRACE_ERROR, &state, exidx, exidx_sz);
+	print_stack_arm32(TRACE_ERROR, &state, exidx, exidx_sz,
+			  false /*!kernel_stack*/, stack, stack_size);
 }
 #endif /* ARM32 */
 #ifdef ARM64
@@ -138,6 +155,7 @@ static void __print_stack_unwind_arm32(struct abort_info *ai __unused)
 static void __print_stack_unwind_arm64(struct abort_info *ai)
 {
 	struct unwind_state_arm64 state;
+	bool kernel_stack;
 	uaddr_t stack;
 	size_t stack_size;
 
@@ -152,17 +170,19 @@ static void __print_stack_unwind_arm64(struct abort_info *ai)
 		/* User stack */
 		stack = utc->stack_addr;
 		stack_size = utc->mobj_stack->size;
+		kernel_stack = false;
 	} else {
 		/* Kernel stack */
 		stack = thread_stack_start();
 		stack_size = thread_stack_size();
+		kernel_stack = true;
 	}
 
 	memset(&state, 0, sizeof(state));
 	state.pc = ai->regs->elr;
 	state.fp = ai->regs->x29;
 
-	print_stack_arm64(TRACE_ERROR, &state, stack, stack_size);
+	print_stack_arm64(TRACE_ERROR, &state, kernel_stack, stack, stack_size);
 }
 #else
 static void __print_stack_unwind_arm64(struct abort_info *ai __unused)
diff --git a/core/arch/arm/kernel/unwind_arm32.c b/core/arch/arm/kernel/unwind_arm32.c
index f1239334..ac38e782 100644
--- a/core/arch/arm/kernel/unwind_arm32.c
+++ b/core/arch/arm/kernel/unwind_arm32.c
@@ -33,9 +33,11 @@
 #include <arm.h>
 #include <kernel/linker.h>
 #include <kernel/misc.h>
+#include <kernel/tee_misc.h>
 #include <kernel/unwind.h>
 #include <string.h>
 #include <tee_api_types.h>
+#include <tee/tee_svc.h>
 #include <trace.h>
 
 /* The register names */
@@ -98,6 +100,15 @@ struct unwind_idx {
 	uint32_t insn;
 };
 
+static bool copy_in(void *dst, const void *src, size_t n, bool kernel_data)
+{
+	if (!kernel_data)
+		return !tee_svc_copy_from_user(dst, src, n);
+
+	memcpy(dst, src, n);
+	return true;
+}
+
 /* Expand a 31-bit signed value to a 32-bit signed value */
 static int32_t expand_prel31(uint32_t prel31)
 {
@@ -144,37 +155,53 @@ static struct unwind_idx *find_index(uint32_t addr, vaddr_t exidx,
 }
 
 /* Reads the next byte from the instruction list */
-static uint8_t unwind_exec_read_byte(struct unwind_state_arm32 *state)
+static bool unwind_exec_read_byte(struct unwind_state_arm32 *state,
+				  uint32_t *ret_insn, bool kernel_stack)
 {
-	uint8_t insn;
+	uint32_t insn;
+
+	if (!copy_in(&insn, (void *)state->insn, sizeof(insn), kernel_stack))
+		return false;
 
 	/* Read the unwind instruction */
-	insn = (*state->insn) >> (state->byte * 8);
+	*ret_insn = (insn >> (state->byte * 8)) & 0xff;
 
 	/* Update the location of the next instruction */
 	if (state->byte == 0) {
 		state->byte = 3;
-		state->insn++;
+		state->insn += sizeof(uint32_t);
 		state->entries--;
 	} else
 		state->byte--;
 
-	return insn;
+	return true;
+}
+
+static bool pop_vsp(uint32_t *reg, vaddr_t *vsp, bool kernel_stack,
+		    vaddr_t stack, size_t stack_size)
+{
+	if (!core_is_buffer_inside(*vsp, sizeof(*reg), stack, stack_size)) {
+		DMSG("vsp out of bounds %#" PRIxVA, *vsp);
+		return false;
+	}
+	if (!copy_in(reg, (void *)*vsp, sizeof(*reg), kernel_stack))
+		return false;
+	(*vsp) += sizeof(*reg);
+	return true;
 }
 
 /* Executes the next instruction on the list */
-static bool unwind_exec_insn(struct unwind_state_arm32 *state)
+static bool unwind_exec_insn(struct unwind_state_arm32 *state,
+			     bool kernel_stack, vaddr_t stack,
+			     size_t stack_size)
 {
-	unsigned int insn;
-	uint32_t *vsp = (uint32_t *)(uintptr_t)state->registers[SP];
+	uint32_t insn;
+	vaddr_t vsp = state->registers[SP];
 	int update_vsp = 0;
 
-	/* This should never happen */
-	if (state->entries == 0)
-		return false;
-
 	/* Read the next instruction */
-	insn = unwind_exec_read_byte(state);
+	if (!unwind_exec_read_byte(state, &insn, kernel_stack))
+		return false;
 
 	if ((insn & INSN_VSP_MASK) == INSN_VSP_INC) {
 		state->registers[SP] += ((insn & INSN_VSP_SIZE_MASK) << 2) + 4;
@@ -183,10 +210,12 @@ static bool unwind_exec_insn(struct unwind_state_arm32 *state)
 		state->registers[SP] -= ((insn & INSN_VSP_SIZE_MASK) << 2) + 4;
 
 	} else if ((insn & INSN_STD_MASK) == INSN_POP_MASKED) {
-		unsigned int mask, reg;
+		uint32_t mask;
+		unsigned int reg;
 
 		/* Load the mask */
-		mask = unwind_exec_read_byte(state);
+		if (!unwind_exec_read_byte(state, &mask, kernel_stack))
+			return false;
 		mask |= (insn & INSN_STD_DATA_MASK) << 8;
 
 		/* We have a refuse to unwind instruction */
@@ -199,7 +228,9 @@ static bool unwind_exec_insn(struct unwind_state_arm32 *state)
 		/* Load the registers */
 		for (reg = 4; mask && reg < 16; mask >>= 1, reg++) {
 			if (mask & 1) {
-				state->registers[reg] = *vsp++;
+				if (!pop_vsp(&state->registers[reg], &vsp,
+					     kernel_stack, stack, stack_size))
+					return false;
 				state->update_mask |= 1 << reg;
 
 				/* If we have updated SP kep its value */
@@ -226,13 +257,17 @@ static bool unwind_exec_insn(struct unwind_state_arm32 *state)
 
 		/* Pop the registers */
 		for (reg = 4; reg <= 4 + count; reg++) {
-			state->registers[reg] = *vsp++;
+			if (!pop_vsp(&state->registers[reg], &vsp,
+				     kernel_stack, stack, stack_size))
+				return false;
 			state->update_mask |= 1 << reg;
 		}
 
 		/* Check if we are in the pop r14 version */
 		if ((insn & INSN_POP_TYPE_MASK) != 0) {
-			state->registers[14] = *vsp++;
+			if (!pop_vsp(&state->registers[14], &vsp, kernel_stack,
+				     stack, stack_size))
+				return false;
 		}
 
 	} else if (insn == INSN_FINISH) {
@@ -240,9 +275,11 @@ static bool unwind_exec_insn(struct unwind_state_arm32 *state)
 		state->entries = 0;
 
 	} else if (insn == INSN_POP_REGS) {
-		unsigned int mask, reg;
+		uint32_t mask;
+		unsigned int reg;
 
-		mask = unwind_exec_read_byte(state);
+		if (!unwind_exec_read_byte(state, &mask, kernel_stack))
+			return false;
 		if (mask == 0 || (mask & 0xf0) != 0)
 			return false;
 
@@ -252,16 +289,19 @@ static bool unwind_exec_insn(struct unwind_state_arm32 *state)
 		/* Load the registers */
 		for (reg = 0; mask && reg < 4; mask >>= 1, reg++) {
 			if (mask & 1) {
-				state->registers[reg] = *vsp++;
+				if (!pop_vsp(&state->registers[reg], &vsp,
+					     kernel_stack, stack, stack_size))
+					return false;
 				state->update_mask |= 1 << reg;
 			}
 		}
 
 	} else if ((insn & INSN_VSP_LARGE_INC_MASK) == INSN_VSP_LARGE_INC) {
-		unsigned int uleb128;
+		uint32_t uleb128;
 
 		/* Read the increment value */
-		uleb128 = unwind_exec_read_byte(state);
+		if (!unwind_exec_read_byte(state, &uleb128, kernel_stack))
+			return false;
 
 		state->registers[SP] += 0x204 + (uleb128 << 2);
 
@@ -271,37 +311,43 @@ static bool unwind_exec_insn(struct unwind_state_arm32 *state)
 		return false;
 	}
 
-	if (update_vsp) {
-		state->registers[SP] = (uint32_t)(uintptr_t)vsp;
-	}
+	if (update_vsp)
+		state->registers[SP] = vsp;
 
 	return true;
 }
 
 /* Performs the unwind of a function */
-static bool unwind_tab(struct unwind_state_arm32 *state)
+static bool unwind_tab(struct unwind_state_arm32 *state, bool kernel_stack,
+		       vaddr_t stack, size_t stack_size)
 {
 	uint32_t entry;
+	uint32_t insn;
 
 	/* Set PC to a known value */
 	state->registers[PC] = 0;
 
+	if (!copy_in(&insn, (void *)state->insn, sizeof(insn), kernel_stack)) {
+		DMSG("Bad insn addr %p", (void *)state->insn);
+		return true;
+	}
+
 	/* Read the personality */
-	entry = *state->insn & ENTRY_MASK;
+	entry = insn & ENTRY_MASK;
 
 	if (entry == ENTRY_ARM_SU16) {
 		state->byte = 2;
 		state->entries = 1;
 	} else if (entry == ENTRY_ARM_LU16) {
 		state->byte = 1;
-		state->entries = ((*state->insn >> 16) & 0xFF) + 1;
+		state->entries = ((insn >> 16) & 0xFF) + 1;
 	} else {
 		DMSG("Unknown entry: %x\n", entry);
 		return true;
 	}
 
 	while (state->entries > 0) {
-		if (!unwind_exec_insn(state))
+		if (!unwind_exec_insn(state, kernel_stack, stack, stack_size))
 			return true;
 	}
 
@@ -321,12 +367,16 @@ static bool unwind_tab(struct unwind_state_arm32 *state)
 	return false;
 }
 
-bool unwind_stack_arm32(struct unwind_state_arm32 *state, uaddr_t exidx,
-			size_t exidx_sz)
+bool unwind_stack_arm32(struct unwind_state_arm32 *state, vaddr_t exidx,
+			size_t exidx_sz, bool kernel_stack, vaddr_t stack,
+			size_t stack_size)
 {
 	struct unwind_idx *index;
 	bool finished;
 
+	if (!exidx_sz)
+		return false;
+
 	/* Reset the mask of updated registers */
 	state->update_mask = 0;
 
@@ -340,15 +390,15 @@ bool unwind_stack_arm32(struct unwind_state_arm32 *state, uaddr_t exidx,
 	if (index->insn != EXIDX_CANTUNWIND) {
 		if (index->insn & (1U << 31)) {
 			/* The data is within the instruction */
-			state->insn = &index->insn;
+			state->insn = (vaddr_t)&index->insn;
 		} else {
 			/* A prel31 offset to the unwind table */
-			state->insn = (uint32_t *)
-			    ((uintptr_t)&index->insn +
-			     expand_prel31(index->insn));
+			state->insn = (vaddr_t)&index->insn +
+				      expand_prel31(index->insn);
 		}
+
 		/* Run the unwind function */
-		finished = unwind_tab(state);
+		finished = unwind_tab(state, kernel_stack, stack, stack_size);
 	}
 
 	/* This is the top of the stack, finish */
@@ -395,14 +445,16 @@ TEE_Result relocate_exidx(void *exidx, size_t exidx_sz, int32_t offset)
 
 #if defined(CFG_UNWIND) && (TRACE_LEVEL > 0)
 
-void print_stack_arm32(int level, struct unwind_state_arm32 *state, uaddr_t exidx,
-		       size_t exidx_sz)
+void print_stack_arm32(int level, struct unwind_state_arm32 *state,
+		       vaddr_t exidx, size_t exidx_sz, bool kernel_stack,
+		       vaddr_t stack, size_t stack_size)
 {
 	trace_printf_helper_raw(level, true, "Call stack:");
 	do {
 		trace_printf_helper_raw(level, true, " 0x%08" PRIx32,
 					state->registers[PC]);
-	} while (unwind_stack_arm32(state, exidx, exidx_sz));
+	} while (unwind_stack_arm32(state, exidx, exidx_sz,
+				    kernel_stack, stack, stack_size));
 }
 
 #endif
@@ -414,8 +466,10 @@ void print_kernel_stack(int level)
 	struct unwind_state_arm32 state;
 	uaddr_t exidx = (vaddr_t)__exidx_start;
 	size_t exidx_sz = (vaddr_t)__exidx_end - (vaddr_t)__exidx_start;
+	vaddr_t stack = thread_stack_start();
+	size_t stack_size = thread_stack_size();
 
-	memset(state.registers, 0, sizeof(state.registers));
+	memset(&state, 0, sizeof(state));
 	/* r7: Thumb-style frame pointer */
 	state.registers[7] = read_r7();
 	/* r11: ARM-style frame pointer */
@@ -424,7 +478,8 @@ void print_kernel_stack(int level)
 	state.registers[LR] = read_lr();
 	state.registers[PC] = (uint32_t)print_kernel_stack;
 
-	print_stack_arm32(level, &state, exidx, exidx_sz);
+	print_stack_arm32(level, &state, exidx, exidx_sz,
+			  true /*kernel_stack*/, stack, stack_size);
 }
 
 #endif
diff --git a/core/arch/arm/kernel/unwind_arm64.c b/core/arch/arm/kernel/unwind_arm64.c
index baf4e080..28a8d572 100644
--- a/core/arch/arm/kernel/unwind_arm64.c
+++ b/core/arch/arm/kernel/unwind_arm64.c
@@ -30,25 +30,41 @@
  */
 
 #include <arm.h>
-#include <kernel/unwind.h>
 #include <kernel/thread.h>
+#include <kernel/unwind.h>
+#include <kernel/tee_misc.h>
 #include <string.h>
+#include <tee/tee_svc.h>
 #include <trace.h>
 
-bool unwind_stack_arm64(struct unwind_state_arm64 *frame, uaddr_t stack,
-		      size_t stack_size)
+static bool copy_in_reg(uint64_t *reg, vaddr_t addr, bool kernel_data)
 {
-	uint64_t fp;
+	if (!kernel_data)
+		return !tee_svc_copy_from_user(reg, (void *)addr, sizeof(*reg));
+
+	memcpy(reg, (void *)addr, sizeof(*reg));
+	return true;
+}
 
-	fp = frame->fp;
-	if (fp < stack || fp >= stack + stack_size)
+bool unwind_stack_arm64(struct unwind_state_arm64 *frame, bool kernel_stack,
+			vaddr_t stack, size_t stack_size)
+{
+	vaddr_t fp = frame->fp;
+
+	if (!core_is_buffer_inside(fp, sizeof(uint64_t) * 3,
+				   stack, stack_size)) {
+		DMSG("FP out of bounds %#" PRIxVA, fp);
 		return false;
+	}
 
 	frame->sp = fp + 0x10;
 	/* FP to previous frame (X29) */
-	frame->fp = *(uint64_t *)(fp);
+	if (!copy_in_reg(&frame->fp, fp, kernel_stack))
+		return false;
 	/* LR (X30) */
-	frame->pc = *(uint64_t *)(fp + 8) - 4;
+	if (!copy_in_reg(&frame->pc, fp + 8, kernel_stack))
+		return false;
+	frame->pc -= 4;
 
 	return true;
 }
@@ -56,13 +72,13 @@ bool unwind_stack_arm64(struct unwind_state_arm64 *frame, uaddr_t stack,
 #if defined(CFG_UNWIND) && (TRACE_LEVEL > 0)
 
 void print_stack_arm64(int level, struct unwind_state_arm64 *state,
-		       uaddr_t stack, size_t stack_size)
+		       bool kernel_stack, vaddr_t stack, size_t stack_size)
 {
 	trace_printf_helper_raw(level, true, "Call stack:");
 	do {
 		trace_printf_helper_raw(level, true, " 0x%016" PRIx64,
 					state->pc);
-	} while (stack && unwind_stack_arm64(state, stack, stack_size));
+	} while (unwind_stack_arm64(state, kernel_stack, stack, stack_size));
 }
 
 void print_kernel_stack(int level)
@@ -75,7 +91,8 @@ void print_kernel_stack(int level)
 	state.pc = read_pc();
 	state.fp = read_fp();
 
-	print_stack_arm64(level, &state, stack, stack_size);
+	print_stack_arm64(level, &state,
+			  true /*kernel_stack*/, stack, stack_size);
 }
 
 #endif
diff --git a/core/include/tee/tee_svc.h b/core/include/tee/tee_svc.h
index 817da486..e6e1931e 100644
--- a/core/include/tee/tee_svc.h
+++ b/core/include/tee/tee_svc.h
@@ -65,7 +65,17 @@ TEE_Result syscall_invoke_ta_command(unsigned long sess,
 TEE_Result syscall_check_access_rights(unsigned long flags, const void *buf,
 				       size_t len);
 
+#ifdef CFG_WITH_USER_TA
 TEE_Result tee_svc_copy_from_user(void *kaddr, const void *uaddr, size_t len);
+#else
+static inline TEE_Result tee_svc_copy_from_user(void *kaddr __unused,
+						const void *uaddr __unused,
+						size_t len __unused)
+{
+	return TEE_ERROR_NOT_SUPPORTED;
+}
+#endif
+
 TEE_Result tee_svc_copy_to_user(void *uaddr, const void *kaddr, size_t len);
 
 TEE_Result tee_svc_copy_kaddr_to_uref(uint32_t *uref, void *kaddr);