[PATCH] spl: fit: Prefer a malloc()'d buffer for loading images

* [PATCH] spl: fit: Prefer a malloc()'d buffer for loading images
@ 2020-10-21 23:32 Alexandru Gagniuc
  2020-11-03 15:11 ` Simon Glass
  2020-12-08 14:54 ` Tom Rini
  0 siblings, 2 replies; 4+ messages in thread
From: Alexandru Gagniuc @ 2020-10-21 23:32 UTC (permalink / raw)
  To: u-boot

Fit images were loaded to a buffer provided by spl_get_load_buffer().
This may work when the FIT image is small and fits between the start
of DRAM and SYS_TEXT_BASE.

One problem with this approach is that the location of the buffer may
be manipulated by changing the 'size' field of the FIT. A maliciously
crafted FIT image could place the buffer over executable code and be
able to take control of SPL. This is unacceptable for secure boot of
signed FIT images.

Another problem is with larger FIT images, usually containing one or
more linux kernels. In such cases the buffer be be large enough so as
to start before DRAM (Figure I). Trying to load an image in this case
has undefined behavior.
For example, on stm32mp1, the MMC controller hits a RX overrun error,
and aborts loading.
    _________________
   |    FIT Image    |
   |                 |
  /===================\                        /=====================\
  ||      DRAM       ||                        |        DRAM         |
  ||                 ||                        |                     |
  ||_________________||  SYS_TEXT_BASE         | ___________________ |
  |                   |                        ||     FIT Image     ||
  |                   |                        ||                   ||
  | _________________ |  SYS_SPL_MALLOC_START  || _________________ ||
  ||  malloc() data  ||                        |||  malloc() data  |||
  ||_________________||                        |||_________________|||
  |                   |                        ||___________________||
  |                   |                        |                     |

        Figure I                                       Figure II

One possibility that was analyzed was to remove the negative offset,
such that the buffer starts at SYS_TEXT_BASE. This is not a proper
solution because on a number of platforms, the malloc buffer() is
placed at a fixed address, usually after SYS_TEXT_BASE. A large
enough FIT image could cause the malloc()'d data to be overwritten
(Figure II) when loading.

          /======================\
          |        DRAM          |
          |                      |
          |                      |   CONFIG_SYS_TEXT_BASE
          |                      |
          |                      |
          | ____________________ |   CONFIG_SYS_SPL_MALLOC_START
          ||   malloc() data    ||
          ||                    ||
          || __________________ ||
          |||    FIT Image     |||
          |||                  |||
          |||                  |||

                 Figure III

The solution proposed here is to replace the ad-hoc heuristics of
spl_get_load_buffer() with malloc(). This provides two advantages:
    * Bounds checking of the buffer region
    * Guarantees the buffer does not conflict with other memory

The first problem is solved by constraining the buffer such that it
will not overlap currently executing code. This eliminates the chance
of a malicious FIT being able to replace the executing SPL code prior
to signature checking.

The second problem is solved in conjunction with increasing
CONFIG_SYS_SPL_MALLOC_SIZE. Since the SPL malloc() region is
carefully crafted on a per-platform basis, the chances of memory
conflicts are virtually eliminated.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---

Contingency plan:

If the malloc() fails, the backup plan is to use spl_get_load_buffer()
with the negative offset eliminated. This puts the buffer at
CONFIG_SYS_TEXT_BASE as a final measure. We can't use the negative
offset because that would re-introduce the first prooblem.

Expected fallout:

This only affects the FIT image loading path, so no breakage is
expected for default configurations. Most configs have a sufficiently
large SYS_SPL_MALLOC_SIZE, with the most common value at 1 MB. This
adequate for the majority of u-boot FIT images.

No significant breakage is expected.


 common/spl/spl_fit.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index a90d821c82..98a7531f72 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -472,6 +472,23 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os)
 #endif
 }
 
+/*
+ * The purpose of the FIT load buffer is to provide a memory location that is
+ * independent of the load address of any FIT component.
+ */
+static void *spl_get_fit_load_buffer(size_t size)
+{
+	void *buf;
+
+	buf = malloc(size);
+	if (!buf) {
+		pr_err("Could not get FIT buffer of %lu bytes\n", (ulong)size);
+		pr_err("\tcheck CONFIG_SYS_SPL_MALLOC_SIZE\n");
+		buf = spl_get_load_buffer(0, size);
+	}
+	return buf;
+}
+
 /*
  * Weak default function to allow customizing SPL fit loading for load-only
  * use cases by allowing to skip the parsing/processing of the FIT contents
@@ -486,12 +503,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 			struct spl_load_info *info, ulong sector, void *fit)
 {
 	int sectors;
-	ulong size;
+	ulong size, hsize;
 	unsigned long count;
 	struct spl_image_info image_info;
 	int node = -1;
 	int images, ret;
-	int base_offset, hsize, align_len = ARCH_DMA_MINALIGN - 1;
+	int base_offset;
 	int index = 0;
 	int firmware_node;
 
@@ -507,24 +524,14 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 
 	/*
 	 * So far we only have one block of data from the FIT. Read the entire
-	 * thing, including that first block, placing it so it finishes before
-	 * where we will load the image.
-	 *
-	 * Note that we will load the image such that its first byte will be
-	 * at the load address. Since that byte may be part-way through a
-	 * block, we may load the image up to one block before the load
-	 * address. So take account of that here by subtracting an addition
-	 * block length from the FIT start position.
-	 *
-	 * In fact the FIT has its own load address, but we assume it cannot
-	 * be before CONFIG_SYS_TEXT_BASE.
+	 * thing, including that first block.
 	 *
 	 * For FIT with data embedded, data is loaded as part of FIT image.
 	 * For FIT with external data, data is not loaded in this step.
 	 */
-	hsize = (size + info->bl_len + align_len) & ~align_len;
-	fit = spl_get_load_buffer(-hsize, hsize);
 	sectors = get_aligned_image_size(info, size, 0);
+	hsize = sectors * info->bl_len;
+	fit = spl_get_fit_load_buffer(hsize);
 	count = info->read(info, sector, sectors, fit);
 	debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n",
 	      sector, sectors, fit, count, size);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 4+ messages in thread