From d3280beb700321b0ef47b4f61d84667ba501bc61 Mon Sep 17 00:00:00 2001 From: Juan Castillo Date: Thu, 5 Jun 2014 09:45:36 +0100 Subject: [PATCH] Rework incorrect use of assert() and panic() in codebase Assert a valid security state using the macro sec_state_is_valid(). Replace assert() with panic() in those cases that might arise because of runtime errors and not programming errors. Replace panic() with assert() in those cases that might arise because of programming errors. Fixes ARM-software/tf-issues#96 Change-Id: I51e9ef0439fd5ff5e0edfef49050b69804bf14d5 --- bl31/bl31_main.c | 2 +- bl31/context_mgmt.c | 4 ++-- bl31/interrupt_mgmt.c | 2 +- common/bl_common.c | 5 ++--- drivers/arm/gic/arm_gic.c | 2 +- drivers/arm/tzc400/tzc400.c | 9 ++++++++- include/common/bl_common.h | 1 + include/drivers/arm/tzc400.h | 1 + plat/fvp/aarch64/fvp_common.c | 3 ++- plat/fvp/bl31_fvp_setup.c | 4 +++- services/spd/tspd/tspd_common.c | 2 ++ 11 files changed, 24 insertions(+), 11 deletions(-) diff --git a/bl31/bl31_main.c b/bl31/bl31_main.c index 861b3914c..ff3c53b57 100644 --- a/bl31/bl31_main.c +++ b/bl31/bl31_main.c @@ -125,7 +125,7 @@ void bl31_main(void) ******************************************************************************/ void bl31_set_next_image_type(uint32_t security_state) { - assert(security_state == NON_SECURE || security_state == SECURE); + assert(sec_state_is_valid(security_state)); next_image_type = security_state; } diff --git a/bl31/context_mgmt.c b/bl31/context_mgmt.c index 65f12137e..4502e5dcb 100644 --- a/bl31/context_mgmt.c +++ b/bl31/context_mgmt.c @@ -71,7 +71,7 @@ void cm_init(void) ******************************************************************************/ void *cm_get_context_by_mpidr(uint64_t mpidr, uint32_t security_state) { - assert(security_state <= NON_SECURE); + assert(sec_state_is_valid(security_state)); return get_cpu_data_by_mpidr(mpidr, cpu_context[security_state]); } @@ -82,7 +82,7 @@ void *cm_get_context_by_mpidr(uint64_t mpidr, uint32_t security_state) ******************************************************************************/ void cm_set_context_by_mpidr(uint64_t mpidr, void *context, uint32_t security_state) { - assert(security_state <= NON_SECURE); + assert(sec_state_is_valid(security_state)); set_cpu_data_by_mpidr(mpidr, cpu_context[security_state], context); } diff --git a/bl31/interrupt_mgmt.c b/bl31/interrupt_mgmt.c index 2b0c79706..e595634ec 100644 --- a/bl31/interrupt_mgmt.c +++ b/bl31/interrupt_mgmt.c @@ -107,7 +107,7 @@ uint32_t get_scr_el3_from_routing_model(uint32_t security_state) { uint32_t scr_el3; - assert(security_state <= NON_SECURE); + assert(sec_state_is_valid(security_state)); scr_el3 = intr_type_descs[INTR_TYPE_NS].scr_el3[security_state]; scr_el3 |= intr_type_descs[INTR_TYPE_S_EL1].scr_el3[security_state]; scr_el3 |= intr_type_descs[INTR_TYPE_EL3].scr_el3[security_state]; diff --git a/common/bl_common.c b/common/bl_common.c index 60b63f18f..d2c60eff8 100644 --- a/common/bl_common.c +++ b/common/bl_common.c @@ -61,12 +61,11 @@ void change_security_state(unsigned int target_security_state) { unsigned long scr = read_scr(); + assert(sec_state_is_valid(target_security_state)); if (target_security_state == SECURE) scr &= ~SCR_NS_BIT; - else if (target_security_state == NON_SECURE) - scr |= SCR_NS_BIT; else - assert(0); + scr |= SCR_NS_BIT; write_scr(scr); } diff --git a/drivers/arm/gic/arm_gic.c b/drivers/arm/gic/arm_gic.c index 636348baa..86aaa9a1f 100644 --- a/drivers/arm/gic/arm_gic.c +++ b/drivers/arm/gic/arm_gic.c @@ -322,7 +322,7 @@ uint32_t arm_gic_interrupt_type_to_line(uint32_t type, type == INTR_TYPE_EL3 || type == INTR_TYPE_NS); - assert(security_state == NON_SECURE || security_state == SECURE); + assert(sec_state_is_valid(security_state)); /* * We ignore the security state parameter under the assumption that diff --git a/drivers/arm/tzc400/tzc400.c b/drivers/arm/tzc400/tzc400.c index c1716db44..715ea6c0b 100644 --- a/drivers/arm/tzc400/tzc400.c +++ b/drivers/arm/tzc400/tzc400.c @@ -103,7 +103,7 @@ static uint32_t tzc_get_gate_keeper(uint64_t base, uint8_t filter) tmp = (tzc_read_gate_keeper(base) >> GATE_KEEPER_OS_SHIFT) & GATE_KEEPER_OS_MASK; - return tmp >> filter; + return (tmp >> filter) & GATE_KEEPER_FILTER_MASK; } /* This function is not MP safe. */ @@ -241,6 +241,13 @@ void tzc_enable_filters(const tzc_instance_t *controller) for (filter = 0; filter < controller->num_filters; filter++) { state = tzc_get_gate_keeper(controller->base, filter); if (state) { + /* The TZC filter is already configured. Changing the + * programmer's view in an active system can cause + * unpredictable behavior therefore panic for now rather + * than try to determine whether this is safe in this + * instance. See: + * http://infocenter.arm.com/help/index.jsp?\ + * topic=/com.arm.doc.ddi0504c/CJHHECBF.html */ ERROR("TZC : Filter %d Gatekeeper already enabled.\n", filter); panic(); diff --git a/include/common/bl_common.h b/include/common/bl_common.h index e996fd6aa..9945e3a38 100644 --- a/include/common/bl_common.h +++ b/include/common/bl_common.h @@ -33,6 +33,7 @@ #define SECURE 0x0 #define NON_SECURE 0x1 +#define sec_state_is_valid(s) (((s) == SECURE) || ((s) == NON_SECURE)) #define UP 1 #define DOWN 0 diff --git a/include/drivers/arm/tzc400.h b/include/drivers/arm/tzc400.h index b4aa3ba59..03fce546b 100644 --- a/include/drivers/arm/tzc400.h +++ b/include/drivers/arm/tzc400.h @@ -90,6 +90,7 @@ #define GATE_KEEPER_OS_MASK 0xf #define GATE_KEEPER_OR_SHIFT 0 #define GATE_KEEPER_OR_MASK 0xf +#define GATE_KEEPER_FILTER_MASK 0x1 /* Speculation is enabled by default. */ #define SPECULATION_CTRL_WRITE_DISABLE (1 << 1) diff --git a/plat/fvp/aarch64/fvp_common.c b/plat/fvp/aarch64/fvp_common.c index 3fe3a218f..a10f4e8a7 100644 --- a/plat/fvp/aarch64/fvp_common.c +++ b/plat/fvp/aarch64/fvp_common.c @@ -237,7 +237,8 @@ uint64_t plat_get_syscnt_freq(void) counter_base_frequency = mmio_read_32(SYS_CNTCTL_BASE + CNTFID_OFF); /* The first entry of the frequency modes table must not be 0 */ - assert(counter_base_frequency != 0); + if (counter_base_frequency == 0) + panic(); return counter_base_frequency; } diff --git a/plat/fvp/bl31_fvp_setup.c b/plat/fvp/bl31_fvp_setup.c index 96f4772a7..683097ac3 100644 --- a/plat/fvp/bl31_fvp_setup.c +++ b/plat/fvp/bl31_fvp_setup.c @@ -92,7 +92,7 @@ entry_point_info_t *bl31_plat_get_next_image_ep_info(uint32_t type) { #if RESET_TO_BL31 - assert(type <= NON_SECURE); + assert(sec_state_is_valid(type)); SET_PARAM_HEAD(&next_image_ep_info, PARAM_EP, VERSION_1, @@ -116,6 +116,8 @@ entry_point_info_t *bl31_plat_get_next_image_ep_info(uint32_t type) #else entry_point_info_t *next_image_info; + assert(sec_state_is_valid(type)); + next_image_info = (type == NON_SECURE) ? bl2_to_bl31_params->bl33_ep_info : bl2_to_bl31_params->bl32_ep_info; diff --git a/services/spd/tspd/tspd_common.c b/services/spd/tspd/tspd_common.c index c497670bd..1b9609f4a 100644 --- a/services/spd/tspd/tspd_common.c +++ b/services/spd/tspd/tspd_common.c @@ -91,6 +91,7 @@ uint64_t tspd_synchronous_sp_entry(tsp_context_t *tsp_ctx) { uint64_t rc; + assert(tsp_ctx != NULL); assert(tsp_ctx->c_rt_ctx == 0); /* Apply the Secure EL1 system register context and switch to it */ @@ -117,6 +118,7 @@ uint64_t tspd_synchronous_sp_entry(tsp_context_t *tsp_ctx) ******************************************************************************/ void tspd_synchronous_sp_exit(tsp_context_t *tsp_ctx, uint64_t ret) { + assert(tsp_ctx != NULL); /* Save the Secure EL1 system register context */ assert(cm_get_context(SECURE) == &tsp_ctx->cpu_ctx); cm_el1_sysregs_context_save(SECURE);