SPM: Refinements on de-privileged FLIH implementation
There is the possibility that an interrupt preempts scheduler.
This may cause the execution contexts - CURRENT_THREAD, isolation
boundaries and PSP/PSPLIM - out-of-sync with each other.
This patch is to solve this issue.
- The EXC_RETURN payload is statically constructed instead of save
on MSP, PSPLIM substitutes it.
- Add critical section around boundaries update and CURRENT_THREAD
update in do_schedule. This makes boundaries and CURRENT_THREAD
always match.
- Add critical section when manipulating stack contexts in
PendSV_Handler
- Push PSP and PSPLIM on MSP for restoring context, EXC_RETURN payload
is not pushed as it's statically defined now
- Address of FLIH function context is decided by checking if current
PSP belongs to the IRQ owner
Change-Id: I0b6996c98e90c5025676c5299024dd4084c8c79c
Signed-off-by: Kevin Peng <kevin.peng@arm.com>
diff --git a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.c b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.c
index da1b367..c9b27a0 100644
--- a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.c
+++ b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.c
@@ -89,6 +89,7 @@
" mov lr, r3 \n"
" cmp r0, r1 \n" /* ctx of curr and next thrd */
" beq v6v7_pendsv_exit \n" /* No schedule if curr = next */
+ " cpsid i \n"
" mrs r2, psp \n"
" subs r2, #32 \n" /* Make room for r4-r11 */
" stm r2!, {r4-r7} \n" /* Save callee registers */
@@ -112,6 +113,7 @@
" ldm r2!, {r4-r7} \n"
" adds r2, #16 \n" /* End of popping r4-r11 */
" msr psp, r2 \n"
+ " cpsie i \n"
"v6v7_pendsv_exit: \n"
" bx lr \n"
);
@@ -126,19 +128,21 @@
"MRS r0, MSP \n"
"MOV r1, lr \n"
"MRS r2, PSP \n"
- "PUSH {r1, r2} \n" /* Orig_exc_return, PSP */
+ "PUSH {r2, r3} \n" /* PSP dummy */
+ "PUSH {r1, r2} \n" /* Orig_exc_return, dummy */
"BL tfm_core_svc_handler \n"
"MOV lr, r0 \n"
- "LDR r1, [sp] \n" /* Original EXC_RETURN */
+ "POP {r1, r2} \n" /* Orig_exc_return, dummy */
"MOVS r2, #8 \n"
"ANDS r0, r2 \n" /* Mode bit */
"ANDS r1, r2 \n"
+ "POP {r2, r3} \n" /* PSP dummy */
"SUBS r0, r1 \n" /* Compare EXC_RETURN values */
"BGT to_flih_func \n"
"BLT from_flih_func \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
"BX lr \n"
"to_flih_func: \n"
+ "PUSH {r2, r3} \n" /* PSP dummy */
"PUSH {r4-r7} \n"
"MOV r4, r8 \n"
"MOV r5, r9 \n"
@@ -156,7 +160,6 @@
"PUSH {r4, r5} \n" /* Seal stack before EXC_RET */
"BX lr \n"
"from_flih_func: \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
"POP {r4, r5} \n" /* Seal stack */
"POP {r4-r7} \n"
"MOV r8, r4 \n"
@@ -164,7 +167,7 @@
"MOV r10, r6 \n"
"MOV r11, r7 \n"
"POP {r4-r7} \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
+ "POP {r1, r2} \n" /* PSP dummy */
"BX lr \n"
);
}
diff --git a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.h b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.h
index 4ee058d..c619cbb 100644
--- a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.h
+++ b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v6m_v7m.h
@@ -20,8 +20,8 @@
#define EXC_RETURN_FPU_FRAME_BASIC (1 << 4)
#endif
-/* Initial EXC_RETURN value in LR when a thread is loaded at the first time */
#define EXC_RETURN_THREAD_S_PSP 0xFFFFFFFD
+#define EXC_RETURN_HANDLER_S_MSP 0xFFFFFFF1
/* Exception return behavior */
diff --git a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_base.c b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_base.c
index fe5bf24..93960be 100644
--- a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_base.c
+++ b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_base.c
@@ -99,6 +99,7 @@
" mov lr, r3 \n"
" cmp r0, r1 \n" /* curr, next ctx */
" beq v8b_pendsv_exit \n" /* No schedule */
+ " cpsid i \n"
" mrs r2, psp \n"
" mrs r3, psplim \n"
" subs r2, #32 \n" /* For r4-r7 */
@@ -124,6 +125,7 @@
" adds r2, #16 \n" /* Pop r4-r11 end */
" msr psp, r2 \n"
" msr psplim, r3 \n"
+ " cpsie i \n"
"v8b_pendsv_exit: \n"
" bx lr \n"
);
@@ -161,19 +163,22 @@
"MRS r0, MSP \n"
"MOV r1, lr \n"
"MRS r2, PSP \n"
- "PUSH {r1, r2} \n" /* Orig_exc_return, PSP */
+ "MRS r3, PSPLIM \n"
+ "PUSH {r2, r3} \n" /* PSP PSPLIM */
+ "PUSH {r1, r2} \n" /* Orig_exc_return, dummy */
"BL tfm_core_svc_handler \n"
"MOV lr, r0 \n"
- "LDR r1, [sp] \n" /* Original EXC_RETURN */
+ "POP {r1, r2} \n" /* Orig_exc_return, dummy */
"MOVS r2, #8 \n"
"ANDS r0, r2 \n" /* Mode bit */
"ANDS r1, r2 \n"
+ "POP {r2, r3} \n" /* PSP PSPLIM */
"SUBS r0, r1 \n" /* Compare EXC_RETURN values */
"BGT to_flih_func \n"
"BLT from_flih_func \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
"BX lr \n"
"to_flih_func: \n"
+ "PUSH {r2, r3} \n" /* PSP PSPLIM */
"PUSH {r4-r7} \n"
"MOV r4, r8 \n"
"MOV r5, r9 \n"
@@ -191,7 +196,6 @@
"PUSH {r4, r5} \n" /* Seal stack before EXC_RET */
"BX lr \n"
"from_flih_func: \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
"POP {r4, r5} \n" /* Seal stack */
"POP {r4-r7} \n"
"MOV r8, r4 \n"
@@ -199,7 +203,7 @@
"MOV r10, r6 \n"
"MOV r11, r7 \n"
"POP {r4-r7} \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
+ "POP {r1, r2} \n" /* PSP PSPLIM */
"BX lr \n"
);
}
diff --git a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_main.c b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_main.c
index 2cfc4bc..472b508 100644
--- a/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_main.c
+++ b/secure_fw/spm/cmsis_psa/arch/tfm_arch_v8m_main.c
@@ -100,6 +100,7 @@
" pop {r2, lr} \n"
" cmp r0, r1 \n" /* curr, next ctx */
" beq v8m_pendsv_exit \n" /* No schedule */
+ " cpsid i \n"
" mrs r2, psp \n"
" mrs r3, psplim \n"
" stmdb r2!, {r4-r11} \n" /* Save callee */
@@ -108,6 +109,7 @@
" ldmia r2!, {r4-r11} \n" /* Restore callee */
" msr psp, r2 \n"
" msr psplim, r3 \n"
+ " cpsie i \n"
"v8m_pendsv_exit: \n"
" bx lr \n"
);
@@ -140,18 +142,21 @@
"MRS r0, MSP \n"
"MOV r1, lr \n"
"MRS r2, PSP \n"
- "PUSH {r1, r2} \n" /* Orig_exc_return, PSP */
+ "MRS r3, PSPLIM \n"
+ "PUSH {r2, r3} \n" /* PSP PSPLIM */
+ "PUSH {r1, r2} \n" /* Orig_exc_return, dummy */
"BL tfm_core_svc_handler \n"
"MOV lr, r0 \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
+ "POP {r1, r2} \n" /* Orig_exc_return, dummy */
+ "POP {r2, r3} \n" /* PSP PSPLIM */
"AND r0, #8 \n" /* Mode bit */
- "AND r3, r1, #8 \n"
- "SUBS r0, r3 \n" /* Compare EXC_RETURN values */
+ "AND r1, #8 \n"
+ "SUBS r0, r1 \n" /* Compare EXC_RETURN values */
"BGT to_flih_func \n"
"BLT from_flih_func \n"
"BX lr \n"
"to_flih_func: \n"
- "PUSH {r1, r2} \n" /* Orig_exc_return, PSP */
+ "PUSH {r2, r3} \n" /* PSP PSPLIM */
"PUSH {r4-r11} \n"
"LDR r4, =0xFEF5EDA5 \n" /* clear r4-r11 */
"MOV r5, r4 \n"
@@ -166,7 +171,7 @@
"from_flih_func: \n"
"POP {r4, r5} \n" /* Seal stack */
"POP {r4-r11} \n"
- "POP {r1, r2} \n" /* Orig_exc_return, PSP */
+ "POP {r1, r2} \n" /* PSP PSPLIM */
"BX lr \n"
);
}
diff --git a/secure_fw/spm/cmsis_psa/spm_ipc.c b/secure_fw/spm/cmsis_psa/spm_ipc.c
index b598c6a..245ce43 100755
--- a/secure_fw/spm/cmsis_psa/spm_ipc.c
+++ b/secure_fw/spm/cmsis_psa/spm_ipc.c
@@ -685,6 +685,7 @@
union returning_contexts_t ret_ctx;
struct partition_t *p_part_curr, *p_part_next;
struct thread_t *pth_next = thrd_next();
+ struct critical_section_t cs = CRITICAL_SECTION_STATIC_INIT;
ret_ctx.ctx.curr = (uint32_t)CURRENT_THREAD->p_context_ctrl;
ret_ctx.ctx.next = (uint32_t)CURRENT_THREAD->p_context_ctrl;
@@ -699,6 +700,7 @@
tfm_core_panic();
}
+ CRITICAL_SECTION_ENTER(cs);
/*
* If required, let the platform update boundary based on its
* implementation. Change privilege, MPU or other configurations.
@@ -714,6 +716,7 @@
ret_ctx.ctx.next = (uint32_t)pth_next->p_context_ctrl;
CURRENT_THREAD = pth_next;
+ CRITICAL_SECTION_LEAVE(cs);
}
/*
diff --git a/secure_fw/spm/ffm/interrupt.c b/secure_fw/spm/ffm/interrupt.c
index 87c0751..920554c 100644
--- a/secure_fw/spm/ffm/interrupt.c
+++ b/secure_fw/spm/ffm/interrupt.c
@@ -8,6 +8,7 @@
#include "interrupt.h"
#include "bitops.h"
+#include "current.h"
#include "tfm_arch.h"
#include "tfm_hal_interrupt.h"
#include "tfm_hal_isolation.h"
@@ -19,7 +20,7 @@
__attribute__((naked))
static psa_flih_result_t tfm_flih_deprivileged_handling(void *p_pt,
uintptr_t fn_flih,
- void *p_context_ctrl)
+ void *curr_component)
{
__ASM volatile("SVC %0 \n"
"BX LR \n"
@@ -53,7 +54,7 @@
uintptr_t flih_func)
{
struct partition_t *p_curr_sp;
- uintptr_t sp_limit, stack;
+ uintptr_t sp_base, sp_limit, curr_stack, ctx_stack;
struct context_ctrl_t flih_ctx_ctrl;
/* Come too early before runtime setup, should not happen. */
@@ -61,32 +62,35 @@
tfm_core_panic();
}
- p_curr_sp = GET_CTX_OWNER(CURRENT_THREAD->p_context_ctrl);
- sp_limit =
- ((struct context_ctrl_t *)p_owner_sp->thrd.p_context_ctrl)->sp_limit;
+ p_curr_sp = GET_CURRENT_COMPONENT();
+ sp_base = LOAD_ALLOCED_STACK_ADDR(p_owner_sp->p_ldinf)
+ + p_owner_sp->p_ldinf->stack_size;
+ sp_limit = LOAD_ALLOCED_STACK_ADDR(p_owner_sp->p_ldinf);
- if (p_owner_sp == p_curr_sp) {
- stack = (uintptr_t)__get_PSP();
+ curr_stack = (uintptr_t)__get_PSP();
+ if (curr_stack < sp_base && curr_stack > sp_limit) {
+ /* The IRQ Partition's stack is being used */
+ ctx_stack = curr_stack;
} else {
- stack = ((struct context_ctrl_t *)p_owner_sp->thrd.p_context_ctrl)->sp;
-
- if (p_owner_sp->p_boundaries != p_curr_sp->p_boundaries) {
- tfm_hal_update_boundaries(p_owner_sp->p_ldinf,
- p_owner_sp->p_boundaries);
- }
-
- /*
- * CURRENT_THREAD->p_context_ctrl is the svc_args[2] on MSP, safe to
- * update it. It is only used to track the owner of the thread data,
- * i.e. the partition that has been interrupted.
- */
- THRD_UPDATE_CUR_CTXCTRL(&(p_owner_sp->ctx_ctrl));
+ ctx_stack =
+ ((struct context_ctrl_t *)p_owner_sp->thrd.p_context_ctrl)->sp;
}
+ if (p_owner_sp->p_boundaries != p_curr_sp->p_boundaries) {
+ tfm_hal_update_boundaries(p_owner_sp->p_ldinf,
+ p_owner_sp->p_boundaries);
+ }
+
+ /*
+ * The CURRENT_COMPONENT has been stored on MSP by the SVC call, safe to
+ * update it.
+ */
+ SET_CURRENT_COMPONENT(p_owner_sp);
+
tfm_arch_init_context(&flih_ctx_ctrl,
flih_func, NULL,
(uintptr_t)tfm_flih_func_return,
- sp_limit, stack);
+ sp_limit, ctx_stack);
(void)tfm_arch_refresh_hardware_context(&flih_ctx_ctrl);
@@ -99,25 +103,24 @@
{
struct partition_t *p_prev_sp, *p_owner_sp;
- p_prev_sp = GET_CTX_OWNER(p_ctx_flih_ret->state_ctx.r2);
- p_owner_sp = GET_CTX_OWNER(CURRENT_THREAD->p_context_ctrl);
+ p_prev_sp = (struct partition_t *)(p_ctx_flih_ret->state_ctx.r2);
+ p_owner_sp = GET_CURRENT_COMPONENT();
if (p_owner_sp->p_boundaries != p_prev_sp->p_boundaries) {
tfm_hal_update_boundaries(p_prev_sp->p_ldinf,
p_prev_sp->p_boundaries);
}
- /* Restore context pointer */
- THRD_UPDATE_CUR_CTXCTRL(p_ctx_flih_ret->state_ctx.r2);
+ /* Restore current component */
+ SET_CURRENT_COMPONENT(p_prev_sp);
- tfm_arch_set_psplim(
- ((struct context_ctrl_t *)CURRENT_THREAD->p_context_ctrl)->sp_limit);
+ tfm_arch_set_psplim(p_ctx_flih_ret->psplim);
__set_PSP(p_ctx_flih_ret->psp);
/* Set FLIH result to the ISR */
p_ctx_flih_ret->state_ctx.r0 = (uint32_t)result;
- return p_ctx_flih_ret->exc_ret;
+ return EXC_RETURN_HANDLER_S_MSP;
}
void spm_handle_interrupt(void *p_pt, struct irq_load_info_t *p_ildi)
@@ -148,7 +151,7 @@
flih_result = tfm_flih_deprivileged_handling(
p_part,
(uintptr_t)p_ildi->flih_func,
- CURRENT_THREAD->p_context_ctrl);
+ GET_CURRENT_COMPONENT());
}
}
diff --git a/secure_fw/spm/include/tfm_arch.h b/secure_fw/spm/include/tfm_arch.h
index f8b3323..c0b5276 100644
--- a/secure_fw/spm/include/tfm_arch.h
+++ b/secure_fw/spm/include/tfm_arch.h
@@ -67,8 +67,8 @@
struct context_flih_ret_t {
uint64_t stack_seal; /* Two words stack seal */
struct tfm_additional_context_t addi_ctx;
- uint32_t exc_ret; /* EXC_RETURN value when interrupt exception ocurrs */
uint32_t psp; /* PSP when interrupt exception ocurrs */
+ uint32_t psplim; /* PSPLIM when interrupt exception ocurrs when */
struct tfm_state_context_t state_ctx; /* ctx on SVC_PREPARE_DEPRIV_FLIH */
};
diff --git a/secure_fw/spm/include/tfm_arch_v8m.h b/secure_fw/spm/include/tfm_arch_v8m.h
index 4039561..7dd9993 100644
--- a/secure_fw/spm/include/tfm_arch_v8m.h
+++ b/secure_fw/spm/include/tfm_arch_v8m.h
@@ -40,6 +40,13 @@
EXC_RETURN_STACK_MAIN | EXC_RETURN_RES0 | \
EXC_RETURN_EXC_SECURE
+#define EXC_RETURN_HANDLER_S_MSP \
+ EXC_RETURN_INDICATOR | EXC_RETURN_RES1 | \
+ EXC_RETURN_SECURE_STACK | EXC_RETURN_STACK_RULE | \
+ EXC_RETURN_FPU_FRAME_BASIC | \
+ EXC_RETURN_STACK_MAIN | EXC_RETURN_RES0 | \
+ EXC_RETURN_EXC_SECURE
+
/* Exception numbers */
#define EXC_NUM_THREAD_MODE (0)
#define EXC_NUM_SVCALL (11)