Merge pull request #669 from sandrine-bailleux-arm/sb/tf-hardening

Minor improvements to harden TF code

5 files changed, 82 insertions, 33 deletions

diff --git a/common/bl_common.c b/common/bl_common.c
index acb2ec62..be56256b 100644
--- a/common/bl_common.c
+++ b/common/bl_common.c
@@ -38,6 +38,7 @@
 #include <io_storage.h>
 #include <platform.h>
 #include <string.h>
+#include <utils.h>
 #include <xlat_tables.h>
 
 uintptr_t page_align(uintptr_t value, unsigned dir)
@@ -59,12 +60,44 @@ static inline unsigned int is_page_aligned (uintptr_t addr) {
 /******************************************************************************
  * 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 otherwise.
+ * 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,
 		uintptr_t addr, size_t size)
 {
-	return (addr >= free_base) && (addr + size <= free_base + free_size);
+	uintptr_t free_end, requested_end;
+
+	/*
+	 * Handle corner cases first.
+	 *
+	 * The order of the 2 tests is important, because if there's no space
+	 * left (i.e. free_size == 0) but we don't ask for any memory
+	 * (i.e. size == 0) then we should report that the memory is free.
+	 */
+	if (size == 0)
+		return 1;	/* A zero-byte region is always free */
+	if (free_size == 0)
+		return 0;
+
+	/*
+	 * Check that the end addresses don't overflow.
+	 * If they do, consider that this memory region is not free, as this
+	 * is an invalid scenario.
+	 */
+	if (check_uptr_overflow(free_base, free_size - 1))
+		return 0;
+	free_end = free_base + (free_size - 1);
+
+	if (check_uptr_overflow(addr, size - 1))
+		return 0;
+	requested_end = addr + (size - 1);
+
+	/*
+	 * Finally, check that the requested memory region lies within the free
+	 * region.
+	 */
+	return (addr >= free_base) && (requested_end <= free_end);
 }
 
 /******************************************************************************
@@ -100,7 +133,8 @@ static unsigned int choose_mem_pos(uintptr_t mem_start, uintptr_t mem_end,
  * Reserve the memory region delimited by 'addr' and 'size'. The extents of free
  * memory are passed in 'free_base' and 'free_size' and they will be updated to
  * reflect the memory usage.
- * The caller must ensure the memory to reserve is free.
+ * The caller must ensure the memory to reserve is free and that the addresses
+ * and sizes passed in arguments are sane.
  *****************************************************************************/
 void reserve_mem(uintptr_t *free_base, size_t *free_size,
 		 uintptr_t addr, size_t size)
@@ -113,8 +147,13 @@ void reserve_mem(uintptr_t *free_base, size_t *free_size,
 	assert(free_size != NULL);
 	assert(is_mem_free(*free_base, *free_size, addr, size));
 
-	pos = choose_mem_pos(*free_base, *free_base + *free_size,
-			     addr, addr + size,
+	if (size == 0) {
+		WARN("Nothing to allocate, requested size is zero\n");
+		return;
+	}
+
+	pos = choose_mem_pos(*free_base, *free_base + (*free_size - 1),
+			     addr, addr + (size - 1),
 			     &discard_size);
 
 	reserved_size = size + discard_size;
@@ -135,10 +174,10 @@ static void dump_load_info(uintptr_t image_load_addr,
 	INFO("Trying to load image at address %p, size = 0x%zx\n",
 		(void *)image_load_addr, image_size);
 	INFO("Current memory layout:\n");
-	INFO("  total region = [%p, %p]\n", (void *)mem_layout->total_base,
-		(void *)(mem_layout->total_base + mem_layout->total_size));
-	INFO("  free region = [%p, %p]\n", (void *)mem_layout->free_base,
-		(void *)(mem_layout->free_base + mem_layout->free_size));
+	INFO("  total region = [base = %p, size = 0x%zx]\n",
+		(void *) mem_layout->total_base, mem_layout->total_size);
+	INFO("  free region = [base = %p, size = 0x%zx]\n",
+		(void *) mem_layout->free_base, mem_layout->free_size);
 }
 
 /* Generic function to return the size of an image */
@@ -248,8 +287,8 @@ int load_image(meminfo_t *mem_layout,
 	/* Check that the memory where the image will be loaded is free */
 	if (!is_mem_free(mem_layout->free_base, mem_layout->free_size,
 			 image_base, image_size)) {
-		WARN("Failed to reserve memory: %p - %p\n", (void *) image_base,
-		     (void *) (image_base + image_size));
+		WARN("Failed to reserve region [base = %p, size = 0x%zx]\n",
+		     (void *) image_base, image_size);
 		dump_load_info(image_base, image_size, mem_layout);
 		io_result = -ENOMEM;
 		goto exit;
@@ -278,8 +317,8 @@ int load_image(meminfo_t *mem_layout,
 				image_base, image_size);
 		entry_point_info->pc = image_base;
 	} else {
-		INFO("Skip reserving memory: %p - %p\n", (void *) image_base,
-		     (void *) (image_base + image_size));
+		INFO("Skip reserving region [base = %p, size = 0x%zx]\n",
+		     (void *) image_base, image_size);
 	}
 
 	/*
@@ -289,8 +328,8 @@ int load_image(meminfo_t *mem_layout,
 	 */
 	flush_dcache_range(image_base, image_size);
 
-	INFO("Image id=%u loaded: %p - %p\n", image_id, (void *) image_base,
-	     (void *) (image_base + image_size));
+	INFO("Image id=%u loaded at address %p, size = 0x%zx\n", image_id,
+		(void *) image_base, image_size);
 
 exit:
 	io_close(image_handle);
diff --git a/common/runtime_svc.c b/common/runtime_svc.c
index 8729e292..7a5855b6 100644
--- a/common/runtime_svc.c
+++ b/common/runtime_svc.c
@@ -54,7 +54,7 @@ static rt_svc_desc_t *rt_svc_descs;
 /*******************************************************************************
  * Simple routine to sanity check a runtime service descriptor before using it
  ******************************************************************************/
-static int32_t validate_rt_svc_desc(rt_svc_desc_t *desc)
+static int32_t validate_rt_svc_desc(const rt_svc_desc_t *desc)
 {
 	if (desc == NULL)
 		return -EINVAL;
@@ -98,18 +98,18 @@ void runtime_svc_init(void)
 
 	rt_svc_descs = (rt_svc_desc_t *) RT_SVC_DESCS_START;
 	for (index = 0; index < RT_SVC_DECS_NUM; index++) {
+		rt_svc_desc_t *service = &rt_svc_descs[index];
 
 		/*
 		 * An invalid descriptor is an error condition since it is
 		 * difficult to predict the system behaviour in the absence
 		 * of this service.
 		 */
-		rc = validate_rt_svc_desc(&rt_svc_descs[index]);
+		rc = validate_rt_svc_desc(service);
 		if (rc) {
-			ERROR("Invalid runtime service descriptor %p (%s)\n",
-					(void *) &rt_svc_descs[index],
-					rt_svc_descs[index].name);
-			goto error;
+			ERROR("Invalid runtime service descriptor %p\n",
+				(void *) service);
+			panic();
 		}
 
 		/*
@@ -119,11 +119,11 @@ void runtime_svc_init(void)
 		 * an initialisation routine defined. Call the initialisation
 		 * routine for this runtime service, if it is defined.
 		 */
-		if (rt_svc_descs[index].init) {
-			rc = rt_svc_descs[index].init();
+		if (service->init) {
+			rc = service->init();
 			if (rc) {
 				ERROR("Error initializing runtime service %s\n",
-						rt_svc_descs[index].name);
+						service->name);
 				continue;
 			}
 		}
@@ -135,15 +135,12 @@ void runtime_svc_init(void)
 		 * entity range.
 		 */
 		start_idx = get_unique_oen(rt_svc_descs[index].start_oen,
-				rt_svc_descs[index].call_type);
+				service->call_type);
+		assert(start_idx < MAX_RT_SVCS);
 		end_idx = get_unique_oen(rt_svc_descs[index].end_oen,
-				rt_svc_descs[index].call_type);
-
+				service->call_type);
+		assert(end_idx < MAX_RT_SVCS);
 		for (; start_idx <= end_idx; start_idx++)
 			rt_svc_descs_indices[start_idx] = index;
 	}
-
-	return;
-error:
-	panic();
 }
diff --git a/include/lib/utils.h b/include/lib/utils.h
index 9cc5468b..0936cbb3 100644
--- a/include/lib/utils.h
+++ b/include/lib/utils.h
@@ -55,4 +55,11 @@
 #define round_down(value, boundary)		\
 	((value) & ~round_boundary(value, boundary))
 
+/*
+ * Evaluates to 1 if (ptr + inc) overflows, 0 otherwise.
+ * Both arguments must be unsigned pointer values (i.e. uintptr_t).
+ */
+#define check_uptr_overflow(ptr, inc)		\
+	(((ptr) > UINTPTR_MAX - (inc)) ? 1 : 0)
+
 #endif /* __UTILS_H__ */
diff --git a/lib/psci/psci_main.c b/lib/psci/psci_main.c
index d412be3c..3ad3dd40 100644
--- a/lib/psci/psci_main.c
+++ b/lib/psci/psci_main.c
@@ -97,6 +97,10 @@ int psci_cpu_suspend(unsigned int power_state,
 			== PSCI_E_SUCCESS);
 
 	target_pwrlvl = psci_find_target_suspend_lvl(&state_info);
+	if (target_pwrlvl == PSCI_INVALID_PWR_LVL) {
+		ERROR("Invalid target power level for suspend operation\n");
+		panic();
+	}
 
 	/* Fast path for CPU standby.*/
 	if (is_cpu_standby_req(is_power_down_state, target_pwrlvl)) {
diff --git a/lib/psci/psci_stat.c b/lib/psci/psci_stat.c
index 155bbb07..ecbe592b 100644
--- a/lib/psci/psci_stat.c
+++ b/lib/psci/psci_stat.c
@@ -259,8 +259,10 @@ int psci_get_stat(u_register_t target_cpu, unsigned int power_state,
 
 	/* Find the highest power level */
 	pwrlvl = psci_find_target_suspend_lvl(&state_info);
-	if (pwrlvl == PSCI_INVALID_PWR_LVL)
-		return PSCI_E_INVALID_PARAMS;
+	if (pwrlvl == PSCI_INVALID_PWR_LVL) {
+		ERROR("Invalid target power level for PSCI statistics operation\n");
+		panic();
+	}
 
 	/* Get the index into the stats array */
 	local_state = state_info.pwr_domain_state[pwrlvl];