From: Antonio Nino Diaz Date: Thu, 1 Jun 2017 12:40:17 +0000 (+0100) Subject: FWU: Check for overlaps when loading images X-Git-Url: http://git.lede-project.org./?a=commitdiff_plain;h=128daee29868a8a4a7cf00508126ea68311fd1cc;p=project%2Fbcm63xx%2Fatf.git FWU: Check for overlaps when loading images Added checks to FWU_SMC_IMAGE_COPY to prevent loading data into a memory region where another image data is already loaded. Without this check, if two images are configured to be loaded in overlapping memory regions, one of them can be loaded and authenticated and the copy function is still able to load data from the second image on top of the first one. Since the first image is still in authenticated state, it can be executed, which could lead to the execution of unauthenticated arbitrary code of the second image. Firmware update documentation updated. Change-Id: Ib6871e569794c8e610a5ea59fe162ff5dcec526c Signed-off-by: Antonio Nino Diaz --- diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index ace364d4..43bb4d2b 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -90,6 +90,124 @@ register_t bl1_fwu_smc_handler(unsigned int smc_fid, SMC_RET1(handle, SMC_UNK); } +/******************************************************************************* + * Utility functions to keep track of the images that are loaded at any time. + ******************************************************************************/ + +#ifdef PLAT_FWU_MAX_SIMULTANEOUS_IMAGES +#define FWU_MAX_SIMULTANEOUS_IMAGES PLAT_FWU_MAX_SIMULTANEOUS_IMAGES +#else +#define FWU_MAX_SIMULTANEOUS_IMAGES 10 +#endif + +static int bl1_fwu_loaded_ids[FWU_MAX_SIMULTANEOUS_IMAGES] = { + [0 ... FWU_MAX_SIMULTANEOUS_IMAGES-1] = INVALID_IMAGE_ID +}; + +/* + * Adds an image_id to the bl1_fwu_loaded_ids array. + * Returns 0 on success, 1 on error. + */ +static int bl1_fwu_add_loaded_id(int image_id) +{ + int i; + + /* Check if the ID is already in the list */ + for (i = 0; i < FWU_MAX_SIMULTANEOUS_IMAGES; i++) { + if (bl1_fwu_loaded_ids[i] == image_id) + return 0; + } + + /* Find an empty slot */ + for (i = 0; i < FWU_MAX_SIMULTANEOUS_IMAGES; i++) { + if (bl1_fwu_loaded_ids[i] == INVALID_IMAGE_ID) { + bl1_fwu_loaded_ids[i] = image_id; + return 0; + } + } + + return 1; +} + +/* + * Removes an image_id from the bl1_fwu_loaded_ids array. + * Returns 0 on success, 1 on error. + */ +static int bl1_fwu_remove_loaded_id(int image_id) +{ + int i; + + /* Find the ID */ + for (i = 0; i < FWU_MAX_SIMULTANEOUS_IMAGES; i++) { + if (bl1_fwu_loaded_ids[i] == image_id) { + bl1_fwu_loaded_ids[i] = INVALID_IMAGE_ID; + return 0; + } + } + + return 1; +} + +/******************************************************************************* + * This function checks if the specified image overlaps another image already + * loaded. It returns 0 if there is no overlap, a negative error code otherwise. + ******************************************************************************/ +static int bl1_fwu_image_check_overlaps(int image_id) +{ + const image_desc_t *image_desc, *checked_image_desc; + const image_info_t *info, *checked_info; + + uintptr_t image_base, image_end; + uintptr_t checked_image_base, checked_image_end; + + checked_image_desc = bl1_plat_get_image_desc(image_id); + checked_info = &checked_image_desc->image_info; + + /* Image being checked mustn't be empty. */ + assert(checked_info->image_size != 0); + + checked_image_base = checked_info->image_base; + checked_image_end = checked_image_base + checked_info->image_size - 1; + /* No need to check for overlaps, it's done in bl1_fwu_image_copy(). */ + + for (int i = 0; i < FWU_MAX_SIMULTANEOUS_IMAGES; i++) { + + /* Don't check image against itself. */ + if (bl1_fwu_loaded_ids[i] == image_id) + continue; + + image_desc = bl1_plat_get_image_desc(bl1_fwu_loaded_ids[i]); + + /* Only check images that are loaded or being loaded. */ + assert (image_desc->state != IMAGE_STATE_RESET); + + info = &image_desc->image_info; + + /* There cannot be overlaps with an empty image. */ + if (info->image_size == 0) + continue; + + image_base = info->image_base; + image_end = image_base + info->image_size - 1; + /* + * Overflows cannot happen. It is checked in + * bl1_fwu_image_copy() when the image goes from RESET to + * COPYING or COPIED. + */ + assert (image_end > image_base); + + /* Check if there are overlaps. */ + if (!(image_end < checked_image_base || + checked_image_end < image_base)) { + VERBOSE("Image with ID %d overlaps existing image with ID %d", + checked_image_desc->image_id, image_desc->image_id); + return -EPERM; + } + } + + return 0; +} + /******************************************************************************* * This function is responsible for copying secure images in AP Secure RAM. ******************************************************************************/ @@ -189,6 +307,13 @@ static int bl1_fwu_image_copy(unsigned int image_id, /* Save the given image size. */ image_desc->image_info.image_size = image_size; + /* Make sure the image doesn't overlap other images. */ + if (bl1_fwu_image_check_overlaps(image_id)) { + image_desc->image_info.image_size = 0; + WARN("BL1-FWU: This image overlaps another one\n"); + return -EPERM; + } + /* * copied_size must be explicitly initialized here because the * FWU code doesn't necessarily do it when it resets the state @@ -213,6 +338,11 @@ static int bl1_fwu_image_copy(unsigned int image_id, return -ENOMEM; } + if (bl1_fwu_add_loaded_id(image_id)) { + WARN("BL1-FWU: Too many images loaded at the same time.\n"); + return -ENOMEM; + } + /* Everything looks sane. Go ahead and copy the block of data. */ dest_addr = image_desc->image_info.image_base + image_desc->copied_size; memcpy((void *) dest_addr, (const void *) image_src, block_size); @@ -290,6 +420,11 @@ static int bl1_fwu_image_auth(unsigned int image_id, return -ENOMEM; } + if (bl1_fwu_add_loaded_id(image_id)) { + WARN("BL1-FWU: Too many images loaded at the same time.\n"); + return -ENOMEM; + } + base_addr = image_src; total_size = image_size; @@ -319,6 +454,13 @@ static int bl1_fwu_image_auth(unsigned int image_id, /* Indicate that image can be copied again*/ image_desc->state = IMAGE_STATE_RESET; } + + /* + * Even if this fails it's ok because the ID isn't in the array. + * The image cannot be in RESET state here, it is checked at the + * beginning of the function. + */ + bl1_fwu_remove_loaded_id(image_id); return -EAUTH; } @@ -475,6 +617,13 @@ static int bl1_fwu_sec_image_done(void **handle, unsigned int flags) assert(EP_GET_EXE(image_desc->ep_info.h.attr) == EXECUTABLE); assert(image_desc->state == IMAGE_STATE_EXECUTED); +#if ENABLE_ASSERTIONS + int rc = bl1_fwu_remove_loaded_id(sec_exec_image_id); + assert(rc == 0); +#else + bl1_fwu_remove_loaded_id(sec_exec_image_id); +#endif + /* Update the flags. */ image_desc->state = IMAGE_STATE_RESET; sec_exec_image_id = INVALID_IMAGE_ID; diff --git a/docs/firmware-update.md b/docs/firmware-update.md index 21872fd4..56ef15cb 100644 --- a/docs/firmware-update.md +++ b/docs/firmware-update.md @@ -211,6 +211,7 @@ for BL1 to pass execution control to BL31. if (source block is in secure memory) return -ENOMEM if (source block is not mapped into BL1) return -ENOMEM if (image_size > free secure memory) return -ENOMEM + if (image overlaps another image) return -EPERM This SMC copies the secure image indicated by `image_id` from non-secure memory to secure memory for later authentication. The image may be copied in a single diff --git a/docs/porting-guide.md b/docs/porting-guide.md index 4d7a5ead..c7b9e89c 100644 --- a/docs/porting-guide.md +++ b/docs/porting-guide.md @@ -354,6 +354,13 @@ be defined: NS_BL2U image identifier, used by BL1 to fetch an image descriptor corresponding to NS_BL2U. +For the the Firmware update capability of TRUSTED BOARD BOOT, the following +macros may also be defined: + +* **#define : PLAT_FWU_MAX_SIMULTANEOUS_IMAGES** + + Total number of images that can be loaded simultaneously. If the platform + doesn't specify any value, it defaults to 10. If a SCP_BL2 image is supported by the platform, the following constants must also be defined: