From 843ddee4aab78dfb940121f5ec28b52f7be5b28a Mon Sep 17 00:00:00 2001 From: Yatharth Kochar Date: Mon, 1 Feb 2016 11:04:46 +0000 Subject: [PATCH] Fix the inconsistencies in bl1_tbbr_image_descs[] This patch fixes inconsistencies in bl1_tbbr_image_descs[] and miscellaneous fixes in Firmware Update code. Following are the changes: * As part of the original FWU changes, a `copied_size` field was added to `image_info_t`. This was a subtle binary compatibility break because it changed the size of the `bl31_params_t` struct, which could cause problems if somebody used different versions of BL2 or BL31, one with the old `image_info_t` and one with the new version. This patch put the `copied_size` within the `image_desc_t`. * EXECUTABLE flag is now stored in `ep_info.h.attr` in place of `image_info.h.attr`, associating it to an entrypoint. * The `image_info.image_base` is only relevant for secure images that are copied from non-secure memory into secure memory. This patch removes initializing `image_base` for non secure images in the bl1_tbbr_image_descs[]. * A new macro `SET_STATIC_PARAM_HEAD` is added for populating bl1_tbbr_image_descs[].ep_info/image_info.h members statically. The version, image_type and image attributes are now populated using this new macro. * Added PLAT_ARM_NVM_BASE and PLAT_ARM_NVM_SIZE to avoid direct usage of V2M_FLASH0_XXX in plat/arm/common/arm_bl1_fwu.c. * Refactoring of code/macros related to SECURE and EXECUTABLE flags. NOTE: PLATFORM PORTS THAT RELY ON THE SIZE OF `image_info_t` OR USE the "EXECUTABLE" BIT WITHIN `image_info_t.h.attr` OR USE THEIR OWN `image_desc_t` ARRAY IN BL1, MAY BE BROKEN BY THIS CHANGE. THIS IS CONSIDERED UNLIKELY. Change-Id: Id4e5989af7bf0ed263d19d3751939da1169b561d --- bl1/bl1_context_mgmt.c | 4 +- bl1/bl1_fwu.c | 42 +++++++++---------- bl1/tbbr/tbbr_img_desc.c | 30 +++++++------ include/common/bl_common.h | 42 +++++++++---------- include/plat/arm/board/common/board_arm_def.h | 5 ++- include/plat/arm/common/arm_def.h | 2 +- include/plat/common/common_def.h | 11 ++--- plat/arm/common/arm_bl1_fwu.c | 11 ++--- plat/arm/common/arm_common.mk | 4 +- 9 files changed, 79 insertions(+), 72 deletions(-) diff --git a/bl1/bl1_context_mgmt.c b/bl1/bl1_context_mgmt.c index 6355190e..bd40608b 100644 --- a/bl1/bl1_context_mgmt.c +++ b/bl1/bl1_context_mgmt.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -74,7 +74,7 @@ void bl1_prepare_next_image(unsigned int image_id) next_bl_ep = &image_desc->ep_info; /* Get the image security state. */ - security_state = GET_SEC_STATE(next_bl_ep->h.attr); + security_state = GET_SECURITY_STATE(next_bl_ep->h.attr); /* Setup the Secure/Non-Secure context if not done already. */ if (!cm_get_context(security_state)) diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index 80ce831a..f3338051 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -135,8 +135,8 @@ static int bl1_fwu_image_copy(unsigned int image_id, } /* Only Normal world is allowed to copy a Secure image. */ - if ((GET_SEC_STATE(flags) == SECURE) || - (GET_SEC_STATE(image_desc->ep_info.h.attr) == NON_SECURE)) { + if ((GET_SECURITY_STATE(flags) == SECURE) || + (GET_SECURITY_STATE(image_desc->ep_info.h.attr) == NON_SECURE)) { WARN("BL1-FWU: Copy not allowed for Non-Secure " "image from Secure-world\n"); return -EPERM; @@ -156,10 +156,10 @@ static int bl1_fwu_image_copy(unsigned int image_id, * If last block is more than expected then * clip the block to the required image size. */ - if (image_desc->image_info.copied_size + block_size > + if (image_desc->copied_size + block_size > image_desc->image_info.image_size) { block_size = image_desc->image_info.image_size - - image_desc->image_info.copied_size; + image_desc->copied_size; WARN("BL1-FWU: Copy argument block_size > remaining image size." " Clipping block_size\n"); } @@ -173,13 +173,13 @@ static int bl1_fwu_image_copy(unsigned int image_id, INFO("BL1-FWU: Continuing image copy in blocks\n"); /* Copy image for given block size. */ - base_addr += image_desc->image_info.copied_size; - image_desc->image_info.copied_size += block_size; + base_addr += image_desc->copied_size; + image_desc->copied_size += block_size; memcpy((void *)base_addr, (const void *)image_src, block_size); flush_dcache_range(base_addr, block_size); /* Update the state if last block. */ - if (image_desc->image_info.copied_size == + if (image_desc->copied_size == image_desc->image_info.image_size) { image_desc->state = IMAGE_STATE_COPIED; INFO("BL1-FWU: Image copy in blocks completed\n"); @@ -234,7 +234,7 @@ static int bl1_fwu_image_copy(unsigned int image_id, INFO("BL1-FWU: Started image copy in blocks\n"); } - image_desc->image_info.copied_size = block_size; + image_desc->copied_size = block_size; } return 0; @@ -257,14 +257,14 @@ static int bl1_fwu_image_auth(unsigned int image_id, if (!image_desc) return -EPERM; - if (GET_SEC_STATE(flags) == SECURE) { + if (GET_SECURITY_STATE(flags) == SECURE) { if (image_desc->state != IMAGE_STATE_RESET) { WARN("BL1-FWU: Authentication from secure world " "while in invalid state\n"); return -EPERM; } } else { - if (GET_SEC_STATE(image_desc->ep_info.h.attr) == SECURE) { + if (GET_SECURITY_STATE(image_desc->ep_info.h.attr) == SECURE) { if (image_desc->state != IMAGE_STATE_COPIED) { WARN("BL1-FWU: Authentication of secure image " "from non-secure world while not in copied state\n"); @@ -369,10 +369,10 @@ static int bl1_fwu_image_execute(unsigned int image_id, * Image is NOT in AUTHENTICATED state. */ if ((!image_desc) || - (GET_SEC_STATE(flags) == SECURE) || - (GET_SEC_STATE(image_desc->ep_info.h.attr) == NON_SECURE) || - (GET_EXEC_STATE(image_desc->image_info.h.attr) == NON_EXECUTABLE) || - (image_desc->state != IMAGE_STATE_AUTHENTICATED)) { + (GET_SECURITY_STATE(flags) == SECURE) || + (GET_SECURITY_STATE(image_desc->ep_info.h.attr) == NON_SECURE) || + (EP_GET_EXE(image_desc->ep_info.h.attr) == NON_EXECUTABLE) || + (image_desc->state != IMAGE_STATE_AUTHENTICATED)) { WARN("BL1-FWU: Execution not allowed due to invalid state/args\n"); return -EPERM; } @@ -402,7 +402,7 @@ static register_t bl1_fwu_image_resume(register_t image_param, { image_desc_t *image_desc; unsigned int resume_sec_state; - unsigned int caller_sec_state = GET_SEC_STATE(flags); + unsigned int caller_sec_state = GET_SECURITY_STATE(flags); /* Get the image descriptor for last executed secure image id. */ image_desc = bl1_plat_get_image_desc(sec_exec_image_id); @@ -417,8 +417,8 @@ static register_t bl1_fwu_image_resume(register_t image_param, assert(image_desc); } - assert(GET_SEC_STATE(image_desc->ep_info.h.attr) == SECURE); - assert(GET_EXEC_STATE(image_desc->image_info.h.attr) == EXECUTABLE); + assert(GET_SECURITY_STATE(image_desc->ep_info.h.attr) == SECURE); + assert(EP_GET_EXE(image_desc->ep_info.h.attr) == EXECUTABLE); if (caller_sec_state == SECURE) { assert(image_desc->state == IMAGE_STATE_EXECUTED); @@ -458,7 +458,7 @@ static int bl1_fwu_sec_image_done(void **handle, unsigned int flags) image_desc_t *image_desc; /* Make sure caller is from the secure world */ - if (GET_SEC_STATE(flags) == NON_SECURE) { + if (GET_SECURITY_STATE(flags) == NON_SECURE) { WARN("BL1-FWU: Image done not allowed from normal world\n"); return -EPERM; } @@ -468,8 +468,8 @@ static int bl1_fwu_sec_image_done(void **handle, unsigned int flags) /* image_desc must correspond to a valid secure executing image */ assert(image_desc); - assert(GET_SEC_STATE(image_desc->ep_info.h.attr) == SECURE); - assert(GET_EXEC_STATE(image_desc->image_info.h.attr) == EXECUTABLE); + assert(GET_SECURITY_STATE(image_desc->ep_info.h.attr) == SECURE); + assert(EP_GET_EXE(image_desc->ep_info.h.attr) == EXECUTABLE); assert(image_desc->state == IMAGE_STATE_EXECUTED); /* Update the flags. */ diff --git a/bl1/tbbr/tbbr_img_desc.c b/bl1/tbbr/tbbr_img_desc.c index 42de8517..7651f1c0 100644 --- a/bl1/tbbr/tbbr_img_desc.c +++ b/bl1/tbbr/tbbr_img_desc.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -35,42 +35,46 @@ image_desc_t bl1_tbbr_image_descs[] = { { .image_id = FWU_CERT_ID, - .image_info.h.attr = SET_EXEC_STATE(NON_EXECUTABLE), + SET_STATIC_PARAM_HEAD(image_info, PARAM_IMAGE_BINARY, + VERSION_1, image_info_t, 0), .image_info.image_base = BL2_BASE, - .ep_info.h.attr = SET_SEC_STATE(SECURE), + SET_STATIC_PARAM_HEAD(ep_info, PARAM_IMAGE_BINARY, + VERSION_1, entry_point_info_t, SECURE), }, #if NS_BL1U_BASE { .image_id = NS_BL1U_IMAGE_ID, - .image_info.h.attr = SET_EXEC_STATE(EXECUTABLE), - .image_info.image_base = NS_BL1U_BASE, - .ep_info.h.attr = SET_SEC_STATE(NON_SECURE), + SET_STATIC_PARAM_HEAD(ep_info, PARAM_EP, + VERSION_1, entry_point_info_t, NON_SECURE | EXECUTABLE), .ep_info.pc = NS_BL1U_BASE, }, #endif #if SCP_BL2U_BASE { .image_id = SCP_BL2U_IMAGE_ID, - .image_info.h.attr = SET_EXEC_STATE(NON_EXECUTABLE), + SET_STATIC_PARAM_HEAD(image_info, PARAM_IMAGE_BINARY, + VERSION_1, image_info_t, 0), .image_info.image_base = SCP_BL2U_BASE, - .ep_info.h.attr = SET_SEC_STATE(SECURE), + SET_STATIC_PARAM_HEAD(ep_info, PARAM_IMAGE_BINARY, + VERSION_1, entry_point_info_t, SECURE), }, #endif #if BL2U_BASE { .image_id = BL2U_IMAGE_ID, - .image_info.h.attr = SET_EXEC_STATE(EXECUTABLE), + SET_STATIC_PARAM_HEAD(image_info, PARAM_EP, + VERSION_1, image_info_t, 0), .image_info.image_base = BL2U_BASE, - .ep_info.h.attr = SET_SEC_STATE(SECURE), + SET_STATIC_PARAM_HEAD(ep_info, PARAM_EP, + VERSION_1, entry_point_info_t, SECURE | EXECUTABLE), .ep_info.pc = BL2U_BASE, }, #endif #if NS_BL2U_BASE { .image_id = NS_BL2U_IMAGE_ID, - .image_info.h.attr = SET_EXEC_STATE(NON_EXECUTABLE), - .image_info.image_base = NS_BL2U_BASE, - .ep_info.h.attr = SET_SEC_STATE(NON_SECURE), + SET_STATIC_PARAM_HEAD(ep_info, PARAM_EP, + VERSION_1, entry_point_info_t, NON_SECURE), }, #endif BL2_IMAGE_DESC, diff --git a/include/common/bl_common.h b/include/common/bl_common.h index e5e6717b..f13dc316 100644 --- a/include/common/bl_common.h +++ b/include/common/bl_common.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013-2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2013-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -53,28 +53,12 @@ #define ENTRY_POINT_INFO_ARGS_OFFSET 0x18 /* The following are used to set/get image attributes. */ -#define EXECUTABLE (0x1) -#define NON_EXECUTABLE (0x0) -#define PARAM_EP_EXECUTE_MASK (0x1) -#define PARAM_EP_EXECUTE_SHIFT (0x1) #define PARAM_EP_SECURITY_MASK (0x1) -#define PARAM_EP_SECURITY_SHIFT (0x0) #define GET_SECURITY_STATE(x) (x & PARAM_EP_SECURITY_MASK) #define SET_SECURITY_STATE(x, security) \ ((x) = ((x) & ~PARAM_EP_SECURITY_MASK) | (security)) -#define GET_EXEC_STATE(x) \ - (((x) >> PARAM_EP_EXECUTE_SHIFT) & PARAM_EP_EXECUTE_MASK) - -#define SET_EXEC_STATE(x) \ - (((x) & PARAM_EP_EXECUTE_MASK) << PARAM_EP_EXECUTE_SHIFT) - -#define GET_SEC_STATE(x) \ - (((x) >> PARAM_EP_SECURITY_SHIFT) & PARAM_EP_SECURITY_MASK) - -#define SET_SEC_STATE(x) \ - (((x) & PARAM_EP_SECURITY_MASK) << PARAM_EP_SECURITY_SHIFT) /* * The following are used for image state attributes. @@ -99,11 +83,17 @@ #define EP_GET_ST(x) (x & EP_ST_MASK) #define EP_SET_ST(x, ee) ((x) = ((x) & ~EP_ST_MASK) | (ee)) -#define PARAM_EP 0x01 -#define PARAM_IMAGE_BINARY 0x02 -#define PARAM_BL31 0x03 +#define EP_EXE_MASK 0x8 +#define NON_EXECUTABLE 0x0 +#define EXECUTABLE 0x8 +#define EP_GET_EXE(x) (x & EP_EXE_MASK) +#define EP_SET_EXE(x, ee) ((x) = ((x) & ~EP_EXE_MASK) | (ee)) + +#define PARAM_EP 0x01 +#define PARAM_IMAGE_BINARY 0x02 +#define PARAM_BL31 0x03 -#define VERSION_1 0x01 +#define VERSION_1 0x01 #define INVALID_IMAGE_ID (0xFFFFFFFF) @@ -114,6 +104,14 @@ (_p)->h.attr = (uint32_t)(_attr) ; \ } while (0) +/* Following is used for populating structure members statically. */ +#define SET_STATIC_PARAM_HEAD(_p, _type, _ver, _p_type, _attr) \ + ._p.h.type = (uint8_t)(_type), \ + ._p.h.version = (uint8_t)(_ver), \ + ._p.h.size = (uint16_t)sizeof(_p_type), \ + ._p.h.attr = (uint32_t)(_attr) + + /******************************************************************************* * Constants to indicate type of exception to the common exception handler. ******************************************************************************/ @@ -224,7 +222,6 @@ typedef struct image_info { param_header_t h; uintptr_t image_base; /* physical address of base of image */ uint32_t image_size; /* bytes read from image file */ - uint32_t copied_size; /* image size copied in blocks */ } image_info_t; /***************************************************************************** @@ -238,6 +235,7 @@ typedef struct image_desc { * Refer IMAGE_STATE_XXX defined above. */ unsigned int state; + uint32_t copied_size; /* image size copied in blocks */ image_info_t image_info; entry_point_info_t ep_info; } image_desc_t; diff --git a/include/plat/arm/board/common/board_arm_def.h b/include/plat/arm/board/common/board_arm_def.h index db2a8dfb..70f6b5b5 100644 --- a/include/plat/arm/board/common/board_arm_def.h +++ b/include/plat/arm/board/common/board_arm_def.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -143,5 +143,8 @@ #define PLAT_ARM_FIP_BASE V2M_FLASH0_BASE #define PLAT_ARM_FIP_MAX_SIZE V2M_FLASH0_SIZE +#define PLAT_ARM_NVM_BASE V2M_FLASH0_BASE +#define PLAT_ARM_NVM_SIZE V2M_FLASH0_SIZE + #endif /* __BOARD_ARM_DEF_H__ */ diff --git a/include/plat/arm/common/arm_def.h b/include/plat/arm/common/arm_def.h index 8d753637..95cb6489 100644 --- a/include/plat/arm/common/arm_def.h +++ b/include/plat/arm/common/arm_def.h @@ -299,7 +299,7 @@ #define BL2U_BASE BL2_BASE #define BL2U_LIMIT BL31_BASE #define NS_BL2U_BASE ARM_NS_DRAM1_BASE -#define NS_BL1U_BASE (V2M_FLASH0_BASE + 0x03EB8000) +#define NS_BL1U_BASE (PLAT_ARM_NVM_BASE + 0x03EB8000) /* * ID of the secure physical generic timer interrupt used by the TSP. diff --git a/include/plat/common/common_def.h b/include/plat/common/common_def.h index 916720c5..9fac9fa2 100644 --- a/include/plat/common/common_def.h +++ b/include/plat/common/common_def.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -72,11 +72,12 @@ #define BL2_IMAGE_DESC { \ .image_id = BL2_IMAGE_ID, \ - .image_info.h.version = VERSION_1, \ - .image_info.h.attr = SET_EXEC_STATE(EXECUTABLE),\ + SET_STATIC_PARAM_HEAD(image_info, PARAM_EP, \ + VERSION_1, image_info_t, 0), \ .image_info.image_base = BL2_BASE, \ - .ep_info.h.attr = SET_SEC_STATE(SECURE), \ - .ep_info.pc = BL2_BASE \ + SET_STATIC_PARAM_HEAD(ep_info, PARAM_EP, \ + VERSION_1, entry_point_info_t, SECURE | EXECUTABLE),\ + .ep_info.pc = BL2_BASE, \ } #endif /* __COMMON_DEF_H__ */ diff --git a/plat/arm/common/arm_bl1_fwu.c b/plat/arm/common/arm_bl1_fwu.c index 9a0d93ad..2a18d341 100644 --- a/plat/arm/common/arm_bl1_fwu.c +++ b/plat/arm/common/arm_bl1_fwu.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -33,11 +33,12 @@ #include #include #include +#include #include /* Struct to keep track of usable memory */ -typedef struct bl1_mem_info{ +typedef struct bl1_mem_info { uintptr_t mem_base; unsigned int mem_size; } bl1_mem_info_t; @@ -58,8 +59,8 @@ bl1_mem_info_t fwu_addr_map_non_secure[] = { .mem_size = ARM_NS_DRAM1_SIZE }, { - .mem_base = V2M_FLASH0_BASE, - .mem_size = V2M_FLASH0_SIZE + .mem_base = PLAT_ARM_NVM_BASE, + .mem_size = PLAT_ARM_NVM_SIZE }, { .mem_size = 0 @@ -79,7 +80,7 @@ int bl1_plat_mem_check(uintptr_t mem_base, /* * Check the given image source and size. */ - if (GET_SEC_STATE(flags) == SECURE) + if (GET_SECURITY_STATE(flags) == SECURE) mmap = fwu_addr_map_secure; else mmap = fwu_addr_map_non_secure; diff --git a/plat/arm/common/arm_common.mk b/plat/arm/common/arm_common.mk index 2647f043..425e0d36 100644 --- a/plat/arm/common/arm_common.mk +++ b/plat/arm/common/arm_common.mk @@ -129,8 +129,8 @@ ifneq (${TRUSTED_BOARD_BOOT},0) PLAT_INCLUDES += -Iinclude/bl1/tbbr - BL1_SOURCES += ${AUTH_SOURCES} \ - bl1/tbbr/tbbr_img_desc.c \ + BL1_SOURCES += ${AUTH_SOURCES} \ + bl1/tbbr/tbbr_img_desc.c \ plat/arm/common/arm_bl1_fwu.c BL2_SOURCES += ${AUTH_SOURCES} -- 2.30.2