From a8fc408cdfcbc3d8a04b493510a54d852f60550d Mon Sep 17 00:00:00 2001 From: Aristo Chen Date: Sun, 5 Jul 2026 03:44:09 +0000 Subject: sandbox: vbe: size firmware1 area to fit the binman fw-update section The firmware1 node in test.dts declares area-size = 0xe00000 (14 MiB) but the binman fw-update section in sandbox_vpl.dtsi is 0x2000000 (32 MiB) and the FIT inside it carries ~16 MiB of external data (spl + u-boot subimages). The FIT therefore extends past the declared firmware area, contradicting the documented contract of vbe_read_fit() that the FIT must fit within @area_size. The mismatch was tolerated because no caller actually bounded the external-data load against area_size. Bring the devicetree in line with the binman section size so the FIT extent stays within the trusted firmware area, in preparation for vbe_read_fit() enforcing that bound. state-offset and version-offset are left as-is; they were already inside the FIT data region and are not exercised by test_vbe_vpl. Reviewed-by: Simon Glass Signed-off-by: Aristo Chen --- arch/sandbox/dts/test.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 0887de4333b..37a36705b36 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>; -- cgit v1.3.1 From a84c7670a698c1d42a6da7e713afdeaaa51eae46 Mon Sep 17 00:00:00 2001 From: Aristo Chen Date: Sun, 5 Jul 2026 03:44:10 +0000 Subject: vbe: bound FIT external-data offset and size before blk_read vbe_read_fit() loads a firmware-phase FIT from the trusted firmware area and then issues a blk_read() to pull in the image, and optionally an FDT, referenced by the FIT image node. The source offset on the device and the read length both come from the FIT's data-position or data-offset property and its data-size property, which live on mutable boot media and can be controlled by an attacker with prior write access to the firmware area. Without a range check the resulting blk_read() can read past the firmware area on the device and, on the non-SPL path, write an attacker-chosen number of blocks past the malloc(aligned_size) FIT buffer into adjacent memory. Only the SPL branch routes through spl_load_simple_fit(), which hashes the data. The external-data block reached from TPL or VPL, and from the bootflow path via abrec_read_bootflow_fw() and vbe_simple_read_bootflow_fw(), runs before any signature or hash check on the loaded phase. Confine the FIT-supplied [load_addr, load_addr + len) window to [addr, addr + area_size] before computing block numbers and lengths, and apply the same constraint to fdt_load_addr and fdt_size. The checks are written in subtraction-only form against the trusted area_size so the comparison itself cannot overflow. Reviewed-by: Simon Glass Signed-off-by: Aristo Chen --- boot/vbe_common.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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; -- cgit v1.3.1 From 9fa00f563bea7ac4a4fda3220e05b21ebd4a242f Mon Sep 17 00:00:00 2001 From: Aristo Chen Date: Sun, 5 Jul 2026 03:44:11 +0000 Subject: test: vbe: cover vbe_read_fit() external-data bounds checks vbe_read_fit() rejects FITs whose external-data window extends past the trusted firmware area on disk by returning -E2BIG. Add two sandbox unit tests that construct synthetic FITs with attacker-controlled data-position and data-size values, write them to mmc1, and assert vbe_read_fit() catches each one before issuing the follow-up blk_read(). vbe_read_fit_oob_position uses a data-position past area_size, which trips the load_addr - addr > area_size clause. vbe_read_fit_oversize_data keeps data-position inside the area but picks a data-size that overruns area_size - (load_addr - addr), tripping the third clause. The two remaining bound clauses stay unreachable from a sandbox test. The load_addr < addr guard trivially holds when addr comes from CONFIG_VAL(TEXT_BASE), which is 0 on sandbox, and the FDT-region bound sits behind a !CONFIG_SANDBOX guard in vbe_read_fit(), so fdt_size stays 0 and that block is skipped in this test environment. The new file follows the existing bootstd VBE test layout and writes the FIT at block 16, past the version and nvdata blocks already used by bootstd_setup_for_tests(). Suggested-by: Simon Glass Reviewed-by: Simon Glass Signed-off-by: Aristo Chen --- test/boot/Makefile | 2 +- test/boot/vbe_read_fit.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 225 insertions(+), 1 deletion(-) create mode 100644 test/boot/vbe_read_fit.c diff --git a/test/boot/Makefile b/test/boot/Makefile index 89538d4f0a6..b8e5f64ac82 100644 --- a/test/boot/Makefile +++ b/test/boot/Makefile @@ -18,7 +18,7 @@ endif 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 + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#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); -- cgit v1.3.1