diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index 61f2adb0a..1cc7daf62 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -41,6 +41,7 @@ #include #include #include +#include #include "bl1_private.h" /* @@ -120,93 +121,72 @@ static int bl1_fwu_image_copy(unsigned int image_id, unsigned int image_size, unsigned int flags) { - uintptr_t base_addr; + uintptr_t dest_addr; + unsigned int remaining; /* Get the image descriptor. */ image_desc_t *image_desc = bl1_plat_get_image_desc(image_id); - - /* Check if we are in correct state. */ - if ((!image_desc) || - ((image_desc->state != IMAGE_STATE_RESET) && - (image_desc->state != IMAGE_STATE_COPYING))) { - WARN("BL1-FWU: Copy not allowed due to invalid state\n"); + if (!image_desc) { + WARN("BL1-FWU: Invalid image ID %u\n", image_id); return -EPERM; } - /* Only Normal world is allowed to copy a Secure image. */ - 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"); + /* + * The request must originate from a non-secure caller and target a + * secure image. Any other scenario is invalid. + */ + if (GET_SECURITY_STATE(flags) == SECURE) { + WARN("BL1-FWU: Copy not allowed from secure world.\n"); + return -EPERM; + } + if (GET_SECURITY_STATE(image_desc->ep_info.h.attr) == NON_SECURE) { + WARN("BL1-FWU: Copy not allowed for non-secure images.\n"); return -EPERM; } - if ((!image_src) || (!block_size)) { + /* Check whether the FWU state machine is in the correct state. */ + if ((image_desc->state != IMAGE_STATE_RESET) && + (image_desc->state != IMAGE_STATE_COPYING)) { + WARN("BL1-FWU: Copy not allowed at this point of the FWU" + " process.\n"); + return -EPERM; + } + + if ((!image_src) || (!block_size) || + check_uptr_overflow(image_src, block_size - 1)) { WARN("BL1-FWU: Copy not allowed due to invalid image source" " or block size\n"); return -ENOMEM; } - /* Get the image base address. */ - base_addr = image_desc->image_info.image_base; - if (image_desc->state == IMAGE_STATE_COPYING) { /* - * If last block is more than expected then - * clip the block to the required image size. + * There must have been at least 1 copy operation for this image + * previously. */ - if (image_desc->copied_size + block_size > - image_desc->image_info.image_size) { - block_size = image_desc->image_info.image_size - - image_desc->copied_size; - WARN("BL1-FWU: Copy argument block_size > remaining image size." - " Clipping block_size\n"); - } - - /* Make sure the image src/size is mapped. */ - if (bl1_plat_mem_check(image_src, block_size, flags)) { - WARN("BL1-FWU: Copy arguments source/size not mapped\n"); - return -ENOMEM; - } + assert(image_desc->copied_size != 0); + /* + * The image size must have been recorded in the 1st copy + * operation. + */ + image_size = image_desc->image_info.image_size; + assert(image_size != 0); + assert(image_desc->copied_size < image_size); INFO("BL1-FWU: Continuing image copy in blocks\n"); - - /* Copy image for given 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->copied_size == - image_desc->image_info.image_size) { - image_desc->state = IMAGE_STATE_COPIED; - INFO("BL1-FWU: Image copy in blocks completed\n"); - } - } else { - /* This means image is in RESET state and ready to be copied. */ - INFO("BL1-FWU: Fresh call to copy an image\n"); - - if (!image_size) { - WARN("BL1-FWU: Copy not allowed due to invalid image size\n"); - return -ENOMEM; - } + } else { /* image_desc->state == IMAGE_STATE_RESET */ + INFO("BL1-FWU: Initial call to copy an image\n"); /* - * If block size is more than total size then - * assume block size as the total image size. + * image_size is relevant only for the 1st copy request, it is + * then ignored for subsequent calls for this image. */ - if (block_size > image_size) { - block_size = image_size; - WARN("BL1-FWU: Copy argument block_size > image size." - " Clipping block_size\n"); - } - - /* Make sure the image src/size is mapped. */ - if (bl1_plat_mem_check(image_src, block_size, flags)) { - WARN("BL1-FWU: Copy arguments source/size not mapped\n"); + if (!image_size) { + WARN("BL1-FWU: Copy not allowed due to invalid image" + " size\n"); return -ENOMEM; } + #if LOAD_IMAGE_V2 /* Check that the image size to load is within limit */ if (image_size > image_desc->image_info.image_max_size) { @@ -214,35 +194,57 @@ static int bl1_fwu_image_copy(unsigned int image_id, return -ENOMEM; } #else - /* Find out how much free trusted ram remains after BL1 load */ - meminfo_t *mem_layout = bl1_plat_sec_mem_layout(); - if ((image_desc->image_info.image_base < mem_layout->free_base) || - (image_desc->image_info.image_base + image_size > - mem_layout->free_base + mem_layout->free_size)) { - WARN("BL1-FWU: Memory not available to copy\n"); + /* + * Check the image will fit into the free trusted RAM after BL1 + * load. + */ + const meminfo_t *mem_layout = bl1_plat_sec_mem_layout(); + if (!is_mem_free(mem_layout->free_base, mem_layout->free_size, + image_desc->image_info.image_base, + image_size)) { + WARN("BL1-FWU: Copy not allowed due to insufficient" + " resources.\n"); return -ENOMEM; } #endif - /* Update the image size. */ + /* Save the given image size. */ image_desc->image_info.image_size = image_size; - /* Copy image for given size. */ - memcpy((void *)base_addr, (const void *)image_src, block_size); - flush_dcache_range(base_addr, block_size); - - /* Update the state. */ - if (block_size == image_size) { - image_desc->state = IMAGE_STATE_COPIED; - INFO("BL1-FWU: Image is copied successfully\n"); - } else { - image_desc->state = IMAGE_STATE_COPYING; - INFO("BL1-FWU: Started image copy in blocks\n"); - } - - image_desc->copied_size = block_size; + /* + * copied_size must be explicitly initialized here because the + * FWU code doesn't necessarily do it when it resets the state + * machine. + */ + image_desc->copied_size = 0; } + /* + * If the given block size is more than the total image size + * then clip the former to the latter. + */ + remaining = image_size - image_desc->copied_size; + if (block_size > remaining) { + WARN("BL1-FWU: Block size is too big, clipping it.\n"); + block_size = remaining; + } + + /* Make sure the source image is mapped in memory. */ + if (bl1_plat_mem_check(image_src, block_size, flags)) { + WARN("BL1-FWU: Source image is not mapped.\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); + flush_dcache_range(dest_addr, block_size); + + image_desc->copied_size += block_size; + image_desc->state = (block_size == remaining) ? + IMAGE_STATE_COPIED : IMAGE_STATE_COPYING; + + INFO("BL1-FWU: Copy operation successful.\n"); return 0; } @@ -293,7 +295,8 @@ static int bl1_fwu_image_auth(unsigned int image_id, base_addr = image_desc->image_info.image_base; total_size = image_desc->image_info.image_size; } else { - if ((!image_src) || (!image_size)) { + if ((!image_src) || (!image_size) || + check_uptr_overflow(image_src, image_size - 1)) { WARN("BL1-FWU: Auth not allowed due to invalid" " image source/size\n"); return -ENOMEM; diff --git a/common/bl_common.c b/common/bl_common.c index 15d5bdee0..47bdad5a8 100644 --- a/common/bl_common.c +++ b/common/bl_common.c @@ -53,14 +53,13 @@ uintptr_t page_align(uintptr_t value, unsigned dir) return value; } -#if !LOAD_IMAGE_V2 /****************************************************************************** * Determine whether the memory region delimited by 'addr' and 'size' is free, * given the extents of free memory. * Return 1 if it is free, 0 if it is not free or if the input values are * invalid. *****************************************************************************/ -static int is_mem_free(uintptr_t free_base, size_t free_size, +int is_mem_free(uintptr_t free_base, size_t free_size, uintptr_t addr, size_t size) { uintptr_t free_end, requested_end; @@ -97,6 +96,7 @@ static int is_mem_free(uintptr_t free_base, size_t free_size, return (addr >= free_base) && (requested_end <= free_end); } +#if !LOAD_IMAGE_V2 /****************************************************************************** * Inside a given memory region, determine whether a sub-region of memory is * closer from the top or the bottom of the encompassing region. Return the diff --git a/docs/firmware-update.md b/docs/firmware-update.md index 97df8cf42..21872fd4e 100644 --- a/docs/firmware-update.md +++ b/docs/firmware-update.md @@ -206,21 +206,31 @@ for BL1 to pass execution control to BL31. if (image_id is non-secure image) return -EPERM if (image_id state is not (RESET or COPYING)) return -EPERM if (secure world caller) return -EPERM + if (image_addr + block_size overflows) return -ENOMEM + if (image destination address + image_size overflows) return -ENOMEM 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 -This SMC copies the secure image indicated by `image_id` into secure memory. The -image may be copied in a single block or multiple blocks. In either case, the -total size of the image must be provided in `image_size` when invoking this SMC -the first time for each image. The `image_addr` and `block_size` specify the -source memory block to copy from. If `block_size` >= the size of the remaining -image to copy, then BL1 completes the copy operation and sets the image state -to COPIED. If there is still more to copy, BL1 sets the image state to COPYING. +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 +block or multiple blocks. In either case, the total size of the image must be +provided in `image_size` when invoking this SMC for the first time for each +image; it is ignored in subsequent calls (if any) for the same image. + +The `image_addr` and `block_size` specify the source memory block to copy from. +The destination address is provided by the platform code. + +If `block_size` is greater than the amount of remaining bytes to copy for this +image then the former is truncated to the latter. The copy operation is then +considered as complete and the FWU state machine transitions to the "COPIED" +state. If there is still more to copy, the FWU state machine stays in or +transitions to the COPYING state (depending on the previous state). + When using multiple blocks, the source blocks do not necessarily need to be in contiguous memory. -BL1 returns from exception to the normal world caller. +Once the SMC is handled, BL1 returns from exception to the normal world caller. ### FWU_SMC_IMAGE_AUTH @@ -347,7 +357,7 @@ a `void *`. The SMC does not return. - - - - - - - - - - - - - - - - - - - - - - - - - - -_Copyright (c) 2015, ARM Limited and Contributors. All rights reserved._ +_Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved._ [Porting Guide]: ./porting-guide.md diff --git a/docs/porting-guide.md b/docs/porting-guide.md index a74966c37..e8486f127 100644 --- a/docs/porting-guide.md +++ b/docs/porting-guide.md @@ -1121,10 +1121,15 @@ The default implementation spins forever. unsigned int flags Return : int -BL1 calls this function while handling FWU copy and authenticate SMCs. The -platform must ensure that the provided `mem_base` and `mem_size` are mapped into -BL1, and that this memory corresponds to either a secure or non-secure memory -region as indicated by the security state of the `flags` argument. +BL1 calls this function while handling FWU related SMCs, more specifically when +copying or authenticating an image. Its responsibility is to ensure that the +region of memory identified by `mem_base` and `mem_size` is mapped in BL1, and +that this memory corresponds to either a secure or non-secure memory region as +indicated by the security state of the `flags` argument. + +This function can safely assume that the value resulting from the addition of +`mem_base` and `mem_size` fits into a `uintptr_t` type variable and does not +overflow. This function must return 0 on success, a non-null error code otherwise. diff --git a/include/common/bl_common.h b/include/common/bl_common.h index 12d5036c8..5076dfd52 100644 --- a/include/common/bl_common.h +++ b/include/common/bl_common.h @@ -361,6 +361,9 @@ CASSERT(sizeof(uintptr_t) == ******************************************************************************/ size_t image_size(unsigned int image_id); +int is_mem_free(uintptr_t free_base, size_t free_size, + uintptr_t addr, size_t size); + #if LOAD_IMAGE_V2 int load_image(unsigned int image_id, image_info_t *image_data); diff --git a/plat/arm/common/arm_bl1_fwu.c b/plat/arm/common/arm_bl1_fwu.c index 2a18d3413..da4107b6a 100644 --- a/plat/arm/common/arm_bl1_fwu.c +++ b/plat/arm/common/arm_bl1_fwu.c @@ -35,7 +35,7 @@ #include #include #include - +#include /* Struct to keep track of usable memory */ typedef struct bl1_mem_info { @@ -76,6 +76,12 @@ int bl1_plat_mem_check(uintptr_t mem_base, assert(mem_base); assert(mem_size); + /* + * The caller of this function is responsible for checking upfront that + * the end address doesn't overflow. We double-check this in debug + * builds. + */ + assert(!check_uptr_overflow(mem_base, mem_size - 1)); /* * Check the given image source and size.