[PATCH v1 1/2] spl: fit: bound the external data size before reading it

[Aristo Chen]

load_simple_fit() loads an image stored as external data by reading it
from the boot device with a transfer sized from the FIT data-size
property. That property is listed in exc_prop[] in image-fit-sig.c, so
it is excluded from the configuration signature and stays under the
control of anyone able to modify the boot medium even when
CONFIG_SPL_FIT_SIGNATURE is enabled. The read happens before
fit_image_verify_with_data() checks the image hash, so an inflated
data-size overruns the destination before the corruption can be
detected. The device-tree overlay path is the sharpest case, because
there the destination is a fixed CONFIG_SPL_LOAD_FIT_APPLY_OVERLAY_BUF_SZ
heap buffer.

Pass the size of the destination into load_simple_fit() and reject an
image whose data does not fit before the read is issued. The data-size
property is read into an int, so a value with bit 31 set arrives as a
negative number, and comparing it as an unsigned quantity rejects that
case as well. The block-aligned transfer length is checked too, because
info->read() moves data-size rounded up to the device block length.

Callers that load into a memory region pass CONFIG_SYS_BOOTM_LEN, the
limit spl_parse_legacy_validate() already applies to legacy images. The
overlay path passes the size of its temporary buffer.

Signed-off-by: Aristo Chen <aristo.chen at canonical.com>
---
 common/spl/spl_fit.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 46ebcabe56a..224fd97d680 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -204,6 +204,9 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  *		If the FIT node does not contain a "load" (address) property,
  *		the image gets loaded to the address pointed to by the
  *		load_addr member in this struct, if load_addr is not 0
+ * @max_size:	maximum number of bytes that may be written to the
+ *		destination; an image whose data exceeds this is rejected
+ *		before it is read from the device
  *
  * Return:	0 on success, -EBADSLT if this image is not the correct phase
  * (for CONFIG_BOOTMETH_VBE_SIMPLE_FW), or another negative error number on
@@ -211,7 +214,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
  */
 static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
 			   const struct spl_fit_info *ctx, int node,
-			   struct spl_image_info *image_info)
+			   struct spl_image_info *image_info, ulong max_size)
 {
 	int offset;
 	size_t length;
@@ -291,6 +294,18 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
 			return 0;
 		}
 
+		/*
+		 * data-size is excluded from the configuration signature (it
+		 * is in the exc_prop[] list in image-fit-sig.c), so it remains
+		 * attacker-controlled even after fit_config_verify() succeeds.
+		 * The image hash is only verified after the device read below,
+		 * so an oversized data-size has to be rejected here. The value
+		 * is stored into an int, so one with bit 31 set arrives as a
+		 * negative number; the unsigned compare rejects that too.
+		 */
+		if ((ulong)len > max_size)
+			goto too_big;
+
 		if (spl_decompression_enabled() &&
 		    (image_comp == IH_COMP_GZIP || image_comp == IH_COMP_LZMA))
 			src_ptr = map_sysmem(ALIGN(CONFIG_SYS_LOAD_ADDR, ARCH_DMA_MINALIGN), len);
@@ -302,6 +317,15 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
 		size = get_aligned_image_size(info, length, offset);
 		read_offset = fit_offset + get_aligned_image_offset(info,
 							    offset);
+
+		/* The block-aligned read size must also fit the destination */
+		if (size > max_size) {
+too_big:
+			printf("%s: FIT image too large (max %lu)\n",
+			       __func__, max_size);
+			return -EFBIG;
+		}
+
 		log_debug("reading from offset %x / %lx size %lx to %p: ",
 			  offset, read_offset, size, src_ptr);
 
@@ -427,7 +451,8 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 		spl_image->fdt_addr = map_sysmem(image_info.load_addr, size);
 		memcpy(spl_image->fdt_addr, gd->fdt_blob, size);
 	} else {
-		ret = load_simple_fit(info, offset, ctx, node, &image_info);
+		ret = load_simple_fit(info, offset, ctx, node, &image_info,
+				      CONFIG_SYS_BOOTM_LEN);
 		if (ret < 0)
 			return ret;
 
@@ -479,7 +504,8 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 			}
 			image_info.load_addr = (ulong)tmpbuffer;
 			ret = load_simple_fit(info, offset, ctx, node,
-					      &image_info);
+					      &image_info,
+					      CONFIG_SPL_LOAD_FIT_APPLY_OVERLAY_BUF_SZ);
 			if (ret == -EBADSLT)
 				continue;
 			else if (ret < 0)
@@ -687,7 +713,8 @@ static int spl_fit_load_fpga(struct spl_fit_info *ctx,
 	warn_deprecated("'fpga' property in config node. Use 'loadables'");
 
 	/* Load the image and set up the fpga_image structure */
-	ret = load_simple_fit(info, offset, ctx, node, &fpga_image);
+	ret = load_simple_fit(info, offset, ctx, node, &fpga_image,
+			      CONFIG_SYS_BOOTM_LEN);
 	if (ret) {
 		printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
 		return ret;
@@ -849,7 +876,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 	}
 
 	/* Load the image and set up the spl_image structure */
-	ret = load_simple_fit(info, offset, &ctx, node, spl_image);
+	ret = load_simple_fit(info, offset, &ctx, node, spl_image,
+			      CONFIG_SYS_BOOTM_LEN);
 	if (ret)
 		return ret;
 
@@ -890,7 +918,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
 			continue;
 
 		image_info.load_addr = 0;
-		ret = load_simple_fit(info, offset, &ctx, node, &image_info);
+		ret = load_simple_fit(info, offset, &ctx, node, &image_info,
+				      CONFIG_SYS_BOOTM_LEN);
 		if (ret < 0 && ret != -EBADSLT) {
 			printf("%s: can't load image loadables index %d (ret = %d)\n",
 			       __func__, index, ret);
-- 
2.43.0