From af8149de7c373fd859175f9989a64915832ee8c0 Mon Sep 17 00:00:00 2001 From: Jens Wiklander Date: Wed, 27 Jun 2018 14:15:15 +0200 Subject: [PATCH] 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 Tested-by: Jerome Forissier (HiKey960 AArch32, Aarch64) Tested-by: Jens Wiklander (Juno, QEMU) Signed-off-by: Jens Wiklander --- core/arch/arm/include/kernel/unwind.h | 21 ++-- core/arch/arm/kernel/abort.c | 36 +++++-- core/arch/arm/kernel/unwind_arm32.c | 139 ++++++++++++++++++-------- core/arch/arm/kernel/unwind_arm64.c | 39 ++++++-- core/include/tee/tee_svc.h | 10 ++ 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 421762922df..7d8f5957976 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 e420aa13eb2..a01e5ad1e7d 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 f1239334166..ac38e782a3a 100644 --- a/core/arch/arm/kernel/unwind_arm32.c +++ b/core/arch/arm/kernel/unwind_arm32.c @@ -33,9 +33,11 @@ #include #include #include +#include #include #include #include +#include #include /* 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 baf4e080a55..28a8d572838 100644 --- a/core/arch/arm/kernel/unwind_arm64.c +++ b/core/arch/arm/kernel/unwind_arm64.c @@ -30,25 +30,41 @@ */ #include -#include #include +#include +#include #include +#include #include -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 817da48669b..e6e1931e3b3 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);