spl: fit: bound the external data size before reading it

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
check is done in two places: an early bail on len > max_size, then a
bail on the block-aligned size > max_size. The size check is the
mathematically binding one because size is len rounded up to the
device block length. The early bail exists so that
get_aligned_image_size() never runs on a hostile len, where its int
arithmetic would invoke signed-integer overflow.

For the overlay path the bound is exact: the caller passes the size
of its temporary buffer. For the firmware, loadables, FDT and FPGA
call sites the destination is wherever the load_addr field points,
with no defined upper limit at the call site. Those callers pass
CONFIG_SYS_BOOTM_LEN as a conservative ceiling, matching the same
limit spl_parse_legacy_validate() already applies to legacy images.
It is not a tight bound on the actual capacity at the destination,
just a cap that rejects implausibly-sized data.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index d89384449b38f652b0ce07b9c42253d2b404826b..18bff7b8d4af75dcf263ab149f4055cd49c22506 100644 (file)
--- 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,23 @@ 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 exc_prop[] in image-fit-sig.c), so it stays attacker
+                * controlled even after fit_config_verify() succeeds. The
+                * image hash is only verified after the device read below, so
+                * an oversized value has to be rejected here.
+                *
+                * Bail out before get_aligned_image_size() runs on a hostile
+                * len: that helper does its arithmetic in int and would
+                * invoke signed-integer overflow on a value close to or above
+                * INT_MAX. The block-aligned check further down is the
+                * mathematically binding one, since size is len rounded up to
+                * the device block length.
+                */
+               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 +322,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);
+
+               /*
+                * info->read() transfers the block-aligned size into the
+                * destination, so this is the bound that actually matters;
+                * len was rejected above only to keep this computation safe.
+                */
+               if (size > max_size)
+                       goto too_big;
+
                log_debug("reading from offset %x / %lx size %lx to %p: ",
                          offset, read_offset, size, src_ptr);
 
@@ -372,6 +401,11 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
        upl_add_image(fit, node, load_addr, length);
 
        return 0;
+
+too_big:
+       printf("%s: FIT image too large (data-size %u, max %lu)\n",
+              __func__, (u32)len, max_size);
+       return -EFBIG;
 }
 
 static bool os_takes_devicetree(uint8_t os)
@@ -427,7 +461,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 +514,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 +723,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 +886,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 +928,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);