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 <antonio.ninodiaz@arm.com>

diff --git a/bl1/bl1_fwu.c b/bl1/bl1_fwu.c
index ace364d422a1367998ced9be3f0022255f6617a2..43bb4d2baed47758466d0a1168405c6059f956b3 100644 (file)
--- 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 21872fd4e5e253d2fb9de6e21c309482e94f2009..56ef15cbb2f487452535b0a8565c594ae1eb1884 100644 (file)
--- 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 4d7a5eadc84bcc00d557ba66b73564a254877848..c7b9e89c8859eb5776c52574ef0a7be07391a7d4 100644 (file)
--- 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: