From 99c5ebafbe7486badd7bf28b28871bae13049301 Mon Sep 17 00:00:00 2001 From: Sandrine Bailleux Date: Tue, 8 Nov 2016 14:27:10 +0000 Subject: [PATCH 1/6] Export is_mem_free() function The is_mem_free() function used to be local to bl_common.c. This patch exports it so that it can be used outside of bl_common.c. Change-Id: I01dcb4229f3a36f56a4724b567c5e6c416dc5e98 Signed-off-by: Sandrine Bailleux --- common/bl_common.c | 4 ++-- include/common/bl_common.h | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) 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/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); From 9f1489e445321edcbf17c4098a474180c504c13d Mon Sep 17 00:00:00 2001 From: Sandrine Bailleux Date: Fri, 11 Nov 2016 15:56:20 +0000 Subject: [PATCH 2/6] Minor refactoring of BL1 FWU code This patch introduces no functional change, it just changes the serial console output. - Improve accuracy of error messages by decoupling some error cases; - Improve comments; - Move declaration of 'mem_layout' local variable closer to where it is used and make it const; - Rename a local variable to clarify whether it is a source or a destination address (base_addr -> dest_addr). Change-Id: I349fcf053e233f316310892211d49e35ef2c39d9 Signed-off-by: Sandrine Bailleux Signed-off-by: Dan Handley --- bl1/bl1_fwu.c | 98 ++++++++++++++++++++++++++++----------------------- 1 file changed, 54 insertions(+), 44 deletions(-) diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index 61f2adb0a..e11fcb552 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -120,24 +120,33 @@ 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; /* 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; + } + + /* 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; } @@ -148,61 +157,61 @@ static int bl1_fwu_image_copy(unsigned int image_id, } /* Get the image base address. */ - base_addr = image_desc->image_info.image_base; + dest_addr = image_desc->image_info.image_base; if (image_desc->state == IMAGE_STATE_COPYING) { + image_size = image_desc->image_info.image_size; /* - * If last block is more than expected then - * clip the block to the required image size. + * If the given block size is more than the total image size + * then clip the former to the latter. */ - 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"); + if (image_desc->copied_size + block_size > image_size) { + WARN("BL1-FWU: Block size is too big, clipping it.\n"); + block_size = image_size - image_desc->copied_size; } - /* Make sure the image src/size is mapped. */ + /* Make sure the source image is mapped in memory. */ if (bl1_plat_mem_check(image_src, block_size, flags)) { - WARN("BL1-FWU: Copy arguments source/size not mapped\n"); + WARN("BL1-FWU: Source image to copy is not mapped.\n"); return -ENOMEM; } INFO("BL1-FWU: Continuing image copy in blocks\n"); /* Copy image for given block size. */ - base_addr += image_desc->copied_size; + dest_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); + memcpy((void *)dest_addr, (const void *)image_src, block_size); + flush_dcache_range(dest_addr, block_size); /* Update the state if last block. */ - if (image_desc->copied_size == - image_desc->image_info.image_size) { + if (image_desc->copied_size == 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"); + } else { /* image_desc->state == IMAGE_STATE_RESET */ + INFO("BL1-FWU: Initial call to copy an image\n"); + /* + * image_size is relevant only for the 1st copy request, it is + * then ignored for subsequent calls for this image. + */ if (!image_size) { - WARN("BL1-FWU: Copy not allowed due to invalid image size\n"); + WARN("BL1-FWU: Copy not allowed due to invalid image" + " size\n"); return -ENOMEM; } /* - * If block size is more than total size then - * assume block size as the total image size. + * If the given block size is more than the total image size + * then clip the former to the latter. */ if (block_size > image_size) { + WARN("BL1-FWU: Block size is too big, clipping it.\n"); 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. */ + /* Make sure the source image is mapped in memory. */ if (bl1_plat_mem_check(image_src, block_size, flags)) { WARN("BL1-FWU: Copy arguments source/size not mapped\n"); return -ENOMEM; @@ -215,23 +224,24 @@ static int bl1_fwu_image_copy(unsigned int image_id, } #else /* Find out how much free trusted ram remains after BL1 load */ - meminfo_t *mem_layout = bl1_plat_sec_mem_layout(); + const 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"); + 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); + /* Copy the block of data. */ + memcpy((void *)dest_addr, (const void *)image_src, block_size); + flush_dcache_range(dest_addr, block_size); - /* Update the state. */ + /* Update the state of the FWU state machine. */ if (block_size == image_size) { image_desc->state = IMAGE_STATE_COPIED; INFO("BL1-FWU: Image is copied successfully\n"); From b38a9e5cd765fa1c7e99f20a8236c3594f2f0562 Mon Sep 17 00:00:00 2001 From: Sandrine Bailleux Date: Mon, 14 Nov 2016 14:56:51 +0000 Subject: [PATCH 3/6] bl1_fwu_image_copy() refactoring This patch refactors the code of the function handling a FWU_AUTH_COPY SMC in BL1. All input validation has been moved upfront so it is now shared between the RESET and COPYING states. Change-Id: I6a86576b9ce3243c401c2474fe06f06687a70e2f Signed-off-by: Sandrine Bailleux Signed-off-by: Dan Handley --- bl1/bl1_fwu.c | 91 +++++++++++++++++++-------------------------------- 1 file changed, 33 insertions(+), 58 deletions(-) diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index e11fcb552..9bd1ba92e 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -121,6 +121,7 @@ static int bl1_fwu_image_copy(unsigned int image_id, unsigned int flags) { uintptr_t dest_addr; + unsigned int remaining; /* Get the image descriptor. */ image_desc_t *image_desc = bl1_plat_get_image_desc(image_id); @@ -156,39 +157,9 @@ static int bl1_fwu_image_copy(unsigned int image_id, return -ENOMEM; } - /* Get the image base address. */ - dest_addr = image_desc->image_info.image_base; - if (image_desc->state == IMAGE_STATE_COPYING) { image_size = image_desc->image_info.image_size; - /* - * If the given block size is more than the total image size - * then clip the former to the latter. - */ - if (image_desc->copied_size + block_size > image_size) { - WARN("BL1-FWU: Block size is too big, clipping it.\n"); - block_size = image_size - image_desc->copied_size; - } - - /* Make sure the source image is mapped in memory. */ - if (bl1_plat_mem_check(image_src, block_size, flags)) { - WARN("BL1-FWU: Source image to copy is not mapped.\n"); - return -ENOMEM; - } - INFO("BL1-FWU: Continuing image copy in blocks\n"); - - /* Copy image for given block size. */ - dest_addr += image_desc->copied_size; - image_desc->copied_size += block_size; - memcpy((void *)dest_addr, (const void *)image_src, block_size); - flush_dcache_range(dest_addr, block_size); - - /* Update the state if last block. */ - if (image_desc->copied_size == image_size) { - image_desc->state = IMAGE_STATE_COPIED; - INFO("BL1-FWU: Image copy in blocks completed\n"); - } } else { /* image_desc->state == IMAGE_STATE_RESET */ INFO("BL1-FWU: Initial call to copy an image\n"); @@ -202,20 +173,6 @@ static int bl1_fwu_image_copy(unsigned int image_id, return -ENOMEM; } - /* - * If the given block size is more than the total image size - * then clip the former to the latter. - */ - if (block_size > image_size) { - WARN("BL1-FWU: Block size is too big, clipping it.\n"); - block_size = image_size; - } - - /* Make sure the source image is mapped in memory. */ - if (bl1_plat_mem_check(image_src, block_size, flags)) { - WARN("BL1-FWU: Copy arguments source/size not mapped\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) { @@ -237,22 +194,40 @@ static int bl1_fwu_image_copy(unsigned int image_id, /* Save the given image size. */ image_desc->image_info.image_size = image_size; - /* Copy the block of data. */ - memcpy((void *)dest_addr, (const void *)image_src, block_size); - flush_dcache_range(dest_addr, block_size); - - /* Update the state of the FWU state machine. */ - 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; } From 1bfb706851c8606aede8f9f391afde1e5339fbf3 Mon Sep 17 00:00:00 2001 From: Sandrine Bailleux Date: Mon, 14 Nov 2016 14:58:05 +0000 Subject: [PATCH 4/6] Add some debug assertions in BL1 FWU copy code These debug assertions sanity check the state of the internal FWU state machine data when resuming an incomplete image copy operation. Change-Id: I38a125b0073658c3e2b4b1bdc623ec221741f43e Signed-off-by: Sandrine Bailleux --- bl1/bl1_fwu.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index 9bd1ba92e..7ef184c11 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -158,7 +158,19 @@ static int bl1_fwu_image_copy(unsigned int image_id, } if (image_desc->state == IMAGE_STATE_COPYING) { + /* + * There must have been at least 1 copy operation for this image + * previously. + */ + 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"); } else { /* image_desc->state == IMAGE_STATE_RESET */ INFO("BL1-FWU: Initial call to copy an image\n"); From 949a52d24eea48a58608645b6536ab7158abcbbb Mon Sep 17 00:00:00 2001 From: Sandrine Bailleux Date: Fri, 11 Nov 2016 16:44:37 +0000 Subject: [PATCH 5/6] Fix integer overflows in BL1 FWU code Before adding a base address and a size to compute the end address of an image to copy or authenticate, check this won't result in an integer overflow. If it does then consider the input arguments are invalid. As a result, bl1_plat_mem_check() can now safely assume the end address (computed as the sum of the base address and size of the memory region) doesn't overflow, as the validation is done upfront in bl1_fwu_image_copy/auth(). A debug assertion has been added nonetheless in the ARM implementation in order to help catching such problems, should bl1_plat_mem_check() be called in a different context in the future. Fixes TFV-1: Malformed Firmware Update SMC can result in copy of unexpectedly large data into secure memory Change-Id: I8b8f8dd4c8777705722c7bd0e8b57addcba07e25 Signed-off-by: Sandrine Bailleux Signed-off-by: Dan Handley --- bl1/bl1_fwu.c | 18 ++++++++++++------ plat/arm/common/arm_bl1_fwu.c | 8 +++++++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c index 7ef184c11..1cc7daf62 100644 --- a/bl1/bl1_fwu.c +++ b/bl1/bl1_fwu.c @@ -41,6 +41,7 @@ #include #include #include +#include #include "bl1_private.h" /* @@ -151,7 +152,8 @@ static int bl1_fwu_image_copy(unsigned int image_id, return -EPERM; } - if ((!image_src) || (!block_size)) { + 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; @@ -192,11 +194,14 @@ static int bl1_fwu_image_copy(unsigned int image_id, return -ENOMEM; } #else - /* Find out how much free trusted ram remains after BL1 load */ + /* + * Check the image will fit into the free trusted RAM after BL1 + * load. + */ const 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)) { + 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; @@ -290,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/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. From 34ba298e094d24dd0fc9559999d00e65b71cc117 Mon Sep 17 00:00:00 2001 From: Sandrine Bailleux Date: Fri, 11 Nov 2016 16:58:59 +0000 Subject: [PATCH 6/6] Improve FWU documentation - Clarify the documentation of the 'FWU_SMC_IMAGE_COPY' SMC in the Firmware Update guide. Also extend the list of pre-conditions to include the additional input validation implemented by previous patches. - Improve documentation of bl1_plat_mem_check() in the porting guide. It now specifies that the generic FWU code protects bl1_plat_mem_check() from integer overflows resulting from the addition of the base address and size passed in arguments. Change-Id: I07b47a3778df7b9c089529b2dd2135707640a91c Signed-off-by: Sandrine Bailleux --- docs/firmware-update.md | 28 +++++++++++++++++++--------- docs/porting-guide.md | 13 +++++++++---- 2 files changed, 28 insertions(+), 13 deletions(-) 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.