Merge pull request #783 from danh-arm/sb/bl1-fwu-copy

bl1/bl1_fwu.c

@@ -41,6 +41,7 @@
 #include <platform_def.h>
 #include <smcc_helpers.h>
 #include <string.h>
+#include <utils.h>
 #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);
-
+	if (!image_desc) {
-	/* Check if we are in correct state. */
+		WARN("BL1-FWU: Invalid image ID %u\n", image_id);
-	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");
 		return -EPERM;
 	}
 
-	/* Only Normal world is allowed to copy a Secure image. */
+	/*
-	if ((GET_SECURITY_STATE(flags) == SECURE) ||
+	 * The request must originate from a non-secure caller and target a
-	    (GET_SECURITY_STATE(image_desc->ep_info.h.attr) == NON_SECURE)) {
+	 * secure image. Any other scenario is invalid.
-		WARN("BL1-FWU: Copy not allowed for Non-Secure "
+	 */
-			 "image from Secure-world\n");
+	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
+		 * There must have been at least 1 copy operation for this image
-		 * clip the block to the required image size.
+		 * previously.
 		 */
-		if (image_desc->copied_size + block_size >
+		assert(image_desc->copied_size != 0);
-			 image_desc->image_info.image_size) {
+		/*
-			block_size = image_desc->image_info.image_size -
+		 * The image size must have been recorded in the 1st copy
-				image_desc->copied_size;
+		 * operation.
-			WARN("BL1-FWU: Copy argument block_size > remaining image size."
+		 */
-				" Clipping block_size\n");
+		image_size = image_desc->image_info.image_size;
-		}
+		assert(image_size != 0);
-
+		assert(image_desc->copied_size < image_size);
-		/* 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;
-		}
 
 		INFO("BL1-FWU: Continuing image copy in blocks\n");
-
+	} else { /* image_desc->state == IMAGE_STATE_RESET */
-		/* Copy image for given block size. */
+		INFO("BL1-FWU: Initial call to copy an image\n");
-		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;
-		}
 
 		/*
-		 * If block size is more than total size then
+		 * image_size is relevant only for the 1st copy request, it is
-		 * assume block size as the total image size.
+		 * then ignored for subsequent calls for this image.
 		 */
-		if (block_size > image_size) {
+		if (!image_size) {
-			block_size = image_size;
+			WARN("BL1-FWU: Copy not allowed due to invalid image"
-			WARN("BL1-FWU: Copy argument block_size > image size."
+				" size\n");
-				" 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;
 		}
+
 #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();
+		 * Check the image will fit into the free trusted RAM after BL1
-		if ((image_desc->image_info.image_base < mem_layout->free_base) ||
+		 * load.
-			 (image_desc->image_info.image_base + image_size >
+		 */
-			  mem_layout->free_base + mem_layout->free_size)) {
+		const meminfo_t *mem_layout = bl1_plat_sec_mem_layout();
-			WARN("BL1-FWU: Memory not available to copy\n");
+		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);
+		 * copied_size must be explicitly initialized here because the
-		flush_dcache_range(base_addr, block_size);
+		 * FWU code doesn't necessarily do it when it resets the state
-
+		 * machine.
-		/* Update the state. */
+		 */
-		if (block_size == image_size) {
+		image_desc->copied_size = 0;
-			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;
 	}
 
+	/*
+	 * 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;

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

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
+This SMC copies the secure image indicated by `image_id` from non-secure memory
-image may be copied in a single block or multiple blocks. In either case, the
+to secure memory for later authentication. The image may be copied in a single
-total size of the image must be provided in `image_size` when invoking this SMC
+block or multiple blocks. In either case, the total size of the image must be
-the first time for each image. The `image_addr` and `block_size` specify the
+provided in `image_size` when invoking this SMC for the first time for each
-source memory block to copy from. If `block_size` >= the size of the remaining
+image; it is ignored in subsequent calls (if any) for the same image.
-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.
+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

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
+BL1 calls this function while handling FWU related SMCs, more specifically when
-platform must ensure that the provided `mem_base` and `mem_size` are mapped into
+copying or authenticating an image. Its responsibility is to ensure that the
-BL1, and that this memory corresponds to either a secure or non-secure memory
+region of memory identified by `mem_base` and `mem_size` is mapped in BL1, and
-region as indicated by the security state of the `flags` argument.
+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.
 

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);

plat/arm/common/arm_bl1_fwu.c

@@ -35,7 +35,7 @@
 #include <plat_arm.h>
 #include <platform_def.h>
 #include <tbbr_img_desc.h>
-
+#include <utils.h>
 
 /* 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.