Merge patch series "vbe: bound FIT external-data reads against the firmware area"

Aristo Chen <[email protected]> says:

vbe_read_fit() loads a firmware-phase FIT from a fixed firmware area on
a block device and then issues a follow-up blk_read() to pull in the
image, and optionally an FDT, referenced by the FIT's image node. The
source offset on the device and the read length both come from the FIT
itself, via data-position or data-offset and data-size. Those properties
live on mutable boot media and can be controlled by an attacker with
write access to the firmware area. On the TPL or VPL path, and on the
bootmeth bootflow path reached via abrec_read_bootflow_fw() and
vbe_simple_read_bootflow_fw(), the follow-up blk_read() runs before any
signature or hash check on the loaded phase.

Patch 1 is a sandbox test-tree preparation. The firmware1 node in
arch/sandbox/dts/test.dts declared area-size = 0xe00000 (14 MiB), but
the binman fw-update section in sandbox_vpl.dtsi is 32 MiB and the FIT
inside it carries ~16 MiB of external data, so the FIT already extended
past the declared area. The mismatch was tolerated because no caller
bounded the external-data load against area_size. Patch 1 raises
area-size to match the binman section size so test_vbe_vpl keeps passing
once the bound is enforced. The patches are ordered so the test is never
broken in the middle of the series.

Patch 2 adds the missing range check, confining the FIT-supplied
[load_addr, load_addr + len) window to [addr, addr + area_size] before
block numbers and lengths are computed, and applying the same constraint
to fdt_load_addr and fdt_size. The check is written in subtraction-only
form against the trusted area_size so the comparison cannot itself
overflow.

Patch 3 adds two sandbox unit tests under test/boot/ that construct
synthetic FITs with out-of-range data-position and oversized data-size,
write them to mmc1, and confirm vbe_read_fit() returns -E2BIG for each
before issuing the follow-up blk_read().

Deferring the external-data blk_read() until after the phase has been
signature-verified would be a stronger structural fix and was discussed
on the v1 thread. Simon confirmed the bounded read is the right first
step and that the verify-then-load change should be a separate series,
so this v3 stays scoped to the bound.

Link: https://lore.kernel.org/r/[email protected]

4 files changed, 242 insertions, 2 deletions

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index fa23b179e38..d24feec5422 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -176,7 +176,7 @@
 			storage = "mmc3";
 			skip-offset = <0x800000>;
 			area-start = <0>;
-			area-size = <0xe00000>;
+			area-size = <0x2000000>;
 			state-offset = <0xdffc00>;
 			state-size = <0x40>;
 			version-offset = <0xdffe00>;
diff --git a/boot/vbe_common.c b/boot/vbe_common.c
index a86986d86e9..844100b7ee6 100644
--- a/boot/vbe_common.c
+++ b/boot/vbe_common.c
@@ -278,6 +278,17 @@ int vbe_read_fit(struct udevice *blk, ulong area_offset, ulong area_size,
 		ulong fdt_offset;
 		void *base_buf, *fdt_base_buf;
 
+		/*
+		 * load_addr and len describe the external data location in
+		 * the FIT, which lives on mutable boot media. Bound them
+		 * against the trusted area_size so the blk_read() below
+		 * cannot read past the firmware area or write an
+		 * attacker-chosen number of blocks past the FIT allocation.
+		 */
+		if (load_addr < addr || load_addr - addr > area_size ||
+		    len > area_size - (load_addr - addr))
+			return log_msg_ret("rng", -E2BIG);
+
 		/* Find the start address to load from */
 		base = ALIGN_DOWN(load_addr, desc->blksz);
 
@@ -321,6 +332,11 @@ int vbe_read_fit(struct udevice *blk, ulong area_offset, ulong area_size,
 
 		/* now the FDT */
 		if (fdt_size) {
+			/* Same constraint as above for the FDT region */
+			if (fdt_load_addr < addr ||
+			    fdt_load_addr - addr > area_size ||
+			    fdt_size > area_size - (fdt_load_addr - addr))
+				return log_msg_ret("rng", -E2BIG);
 			fdt_offset = area_offset + fdt_load_addr - addr;
 			blknum = fdt_offset / desc->blksz;
 			extra = fdt_offset % desc->blksz;
diff --git a/test/boot/Makefile b/test/boot/Makefile
index e6aa0ab7d3e..59a87028704 100644
--- a/test/boot/Makefile
+++ b/test/boot/Makefile
@@ -22,7 +22,7 @@ obj-$(CONFIG_$(PHASE_)FIT_VERITY) += fit_verity.o
 obj-$(CONFIG_MEASURED_BOOT) += measurement.o
 
 ifdef CONFIG_OF_LIVE
-obj-$(CONFIG_BOOTMETH_VBE_SIMPLE) += vbe_simple.o
+obj-$(CONFIG_BOOTMETH_VBE_SIMPLE) += vbe_simple.o vbe_read_fit.o
 endif
 obj-$(CONFIG_BOOTMETH_VBE) += vbe_fixup.o
 
diff --git a/test/boot/vbe_read_fit.c b/test/boot/vbe_read_fit.c
new file mode 100644
index 00000000000..f67de0e7165
--- /dev/null
+++ b/test/boot/vbe_read_fit.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Bounds-check tests for vbe_read_fit()
+ *
+ * vbe_read_fit() pulls a firmware-phase FIT from a trusted firmware area
+ * on a block device. The external-data location and size carried in the
+ * FIT image node are attacker-controllable when the firmware area is on
+ * mutable boot media, so vbe_read_fit() must reject FITs whose external
+ * data extends past @area_size before issuing the follow-up blk_read().
+ *
+ * These tests build small synthetic FITs with deliberately out-of-range
+ * values and confirm vbe_read_fit() returns -E2BIG for each.
+ *
+ * Copyright 2026 Canonical Ltd.
+ * Written by Aristo Chen <[email protected]>
+ */
+
+#include <blk.h>
+#include <dm.h>
+#include <image.h>
+#include <memalign.h>
+#include <mmc.h>
+#include <test/test.h>
+#include <test/ut.h>
+#include <linux/libfdt.h>
+#include "bootstd_common.h"
+#include "../../boot/vbe_common.h"
+
+/*
+ * The synthetic FIT is written to mmc1 starting at block TEST_FIT_BLK.
+ * bootstd_setup_for_tests() uses blocks 4 and 6 (see bootstd_common.h);
+ * block 16 leaves a comfortable gap.
+ */
+#define TEST_FIT_BLK	16
+#define TEST_FIT_OFF	((ulong)TEST_FIT_BLK * MMC_MAX_BLOCK_LEN)
+#define TEST_AREA_SIZE	0x1000
+
+/**
+ * build_fit() - Build a minimal external-data FIT for vbe_read_fit()
+ *
+ * The FIT advertises a single firmware image whose @data-position and
+ * @data-size are passed in directly. Both values are attacker-controlled
+ * in the real threat model.
+ *
+ * @buf:		Destination buffer (must be at least 512 bytes)
+ * @buf_size:		Size of @buf
+ * @data_position:	Value written to the image's data-position property
+ * @data_size:		Value written to the image's data-size property
+ * Returns: 0 on success, libfdt error otherwise
+ */
+static int build_fit(void *buf, size_t buf_size, u32 data_position,
+		     u32 data_size)
+{
+	int ret;
+
+	ret = fdt_create(buf, buf_size);
+	if (ret)
+		return ret;
+	ret = fdt_finish_reservemap(buf);
+	if (ret)
+		return ret;
+
+	ret = fdt_begin_node(buf, "");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_DESC_PROP, "vbe-read-fit test");
+	if (ret)
+		return ret;
+	ret = fdt_property_u32(buf, FIT_TIMESTAMP_PROP, 0);
+	if (ret)
+		return ret;
+
+	ret = fdt_begin_node(buf, "images");
+	if (ret)
+		return ret;
+	ret = fdt_begin_node(buf, "u-boot");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_DESC_PROP, "U-Boot");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_TYPE_PROP, "firmware");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_ARCH_PROP, "sandbox");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_OS_PROP, "u-boot");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_PHASE_PROP, "u-boot");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_COMP_PROP, "none");
+	if (ret)
+		return ret;
+	ret = fdt_property_u32(buf, FIT_DATA_POSITION_PROP, data_position);
+	if (ret)
+		return ret;
+	ret = fdt_property_u32(buf, FIT_DATA_SIZE_PROP, data_size);
+	if (ret)
+		return ret;
+	ret = fdt_end_node(buf);	/* u-boot */
+	if (ret)
+		return ret;
+	ret = fdt_end_node(buf);	/* images */
+	if (ret)
+		return ret;
+
+	ret = fdt_begin_node(buf, "configurations");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_DEFAULT_PROP, "conf-1");
+	if (ret)
+		return ret;
+	ret = fdt_begin_node(buf, "conf-1");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, "compatible", "sandbox");
+	if (ret)
+		return ret;
+	ret = fdt_property_string(buf, FIT_FIRMWARE_PROP, "u-boot");
+	if (ret)
+		return ret;
+	ret = fdt_end_node(buf);	/* conf-1 */
+	if (ret)
+		return ret;
+	ret = fdt_end_node(buf);	/* configurations */
+	if (ret)
+		return ret;
+
+	ret = fdt_end_node(buf);	/* root */
+	if (ret)
+		return ret;
+
+	return fdt_finish(buf);
+}
+
+/**
+ * place_fit_on_mmc() - Write a synthetic FIT to mmc1 and return its blk dev
+ *
+ * @uts: Unit test state
+ * @fit: FIT image to write
+ * @blkp: On success, receives the block udevice for mmc1
+ * Returns: 0 on success, -ve on error
+ */
+static int place_fit_on_mmc(struct unit_test_state *uts, const void *fit,
+			    struct udevice **blkp)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(u8, blkbuf, MMC_MAX_BLOCK_LEN);
+	struct udevice *mmc;
+	struct blk_desc *desc;
+	size_t fit_size = fdt_totalsize(fit);
+	size_t pos;
+	int blknum = TEST_FIT_BLK;
+
+	ut_assertok(uclass_get_device(UCLASS_MMC, 1, &mmc));
+	desc = blk_get_by_device(mmc);
+	if (!desc)
+		return log_msg_ret("desc", -ENODEV);
+
+	for (pos = 0; pos < fit_size; pos += MMC_MAX_BLOCK_LEN, blknum++) {
+		size_t this_blk = min(fit_size - pos,
+				      (size_t)MMC_MAX_BLOCK_LEN);
+
+		memset(blkbuf, '\0', MMC_MAX_BLOCK_LEN);
+		memcpy(blkbuf, (const u8 *)fit + pos, this_blk);
+		if (blk_dwrite(desc, blknum, 1, blkbuf) != 1)
+			return log_msg_ret("wr", -EIO);
+	}
+	*blkp = desc->bdev;
+
+	return 0;
+}
+
+/*
+ * data-position points past area_size: vbe_read_fit() must reject the
+ * FIT with -E2BIG before issuing the external-data blk_read().
+ */
+static int vbe_read_fit_oob_position(struct unit_test_state *uts)
+{
+	u8 fit[1024] __aligned(8);
+	struct udevice *blk;
+	ulong load_addr = 0, len = 0;
+	char *name = NULL;
+	int ret;
+
+	ut_assertok(build_fit(fit, sizeof(fit),
+			      TEST_AREA_SIZE + 0x10, 0x40));
+	ut_assertok(place_fit_on_mmc(uts, fit, &blk));
+
+	ret = vbe_read_fit(blk, TEST_FIT_OFF, TEST_AREA_SIZE,
+			   NULL, &load_addr, &len, &name);
+	ut_asserteq(-E2BIG, ret);
+
+	return 0;
+}
+
+BOOTSTD_TEST(vbe_read_fit_oob_position, UTF_DM | UTF_SCAN_FDT);
+
+/*
+ * data-position is inside the area but data-size pushes the end past
+ * area_size: vbe_read_fit() must reject the FIT with -E2BIG.
+ */
+static int vbe_read_fit_oversize_data(struct unit_test_state *uts)
+{
+	u8 fit[1024] __aligned(8);
+	struct udevice *blk;
+	ulong load_addr = 0, len = 0;
+	char *name = NULL;
+	int ret;
+
+	ut_assertok(build_fit(fit, sizeof(fit),
+			      0x400, TEST_AREA_SIZE));
+	ut_assertok(place_fit_on_mmc(uts, fit, &blk));
+
+	ret = vbe_read_fit(blk, TEST_FIT_OFF, TEST_AREA_SIZE,
+			   NULL, &load_addr, &len, &name);
+	ut_asserteq(-E2BIG, ret);
+
+	return 0;
+}
+
+BOOTSTD_TEST(vbe_read_fit_oversize_data, UTF_DM | UTF_SCAN_FDT);