From 19d8a9e346062aa758392277cd50232f7435a8d6 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Thu, 20 Aug 2026 23:59:33 +0200 Subject: [PATCH] F-9758: validate FDT layout in fdt_check_header, use it in fdt_get_string fdt_get_string() bounded stroffset against size_dt_strings but formed the string-table pointer from off_dt_strings without ever validating either header field against totalsize; a DTB declaring a large off_dt_strings with a small size_dt_strings made every property lookup (fdt_getprop -> fdt_get_string) scan far outside the blob. Validate the structural layout in fdt_check_header() for finalized (FDT_MAGIC) blobs: the reservation map, structure block and string table must sit inside the blob and not overlap, checked in 64-bit so the size fields cannot wrap. fdt_get_string() now requires a valid header before forming the pointer. The SW_MAGIC (in-progress edit) state keeps its existing check, since its layout is different. Test fixtures are adjusted to the validated layout: the two pre-existing fdt_get_string fixtures now set the header fields the lookup relies on, the compatible-test builder sets the magic word and points the reservation map at the canonical empty list right after the header (it pointed into the string table before). --- src/fdt.c | 25 ++++++++++++++ tools/unit-tests/unit-fdt.c | 46 +++++++++++++++++++++++++ tools/unit-tests/unit-t10xx-dts-memac.c | 4 ++- 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/fdt.c b/src/fdt.c index 8b15465d..c11ef965 100644 --- a/src/fdt.c +++ b/src/fdt.c @@ -462,10 +462,24 @@ static int fdt_subnode_offset_namelen(const void *fdt, int offset, int fdt_check_header(const void *fdt) { if (fdt_magic(fdt) == FDT_MAGIC) { + uint32_t off_rsv = fdt_off_mem_rsvmap(fdt); + uint32_t off_struct = fdt_off_dt_struct(fdt); + uint32_t size_struct = fdt_size_dt_struct(fdt); + uint32_t off_strings = fdt_off_dt_strings(fdt); + uint32_t size_strings = fdt_size_dt_strings(fdt); + if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION) return -FDT_ERR_BADVERSION; if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION) return -FDT_ERR_BADVERSION; + /* The three structural areas must sit inside the blob and not + * overlap: reservation map, structure block and string table, + * in that order. The additions are made in 64-bit so the size + * fields cannot wrap around the comparison. */ + if (off_rsv > off_struct + || (uint64_t)off_struct + size_struct > off_strings + || (uint64_t)off_strings + size_strings > fdt_totalsize(fdt)) + return -FDT_ERR_BADSTRUCTURE; } else if (fdt_magic(fdt) == FDT_SW_MAGIC) { if (fdt_size_dt_struct(fdt) == 0) @@ -579,6 +593,17 @@ const char* fdt_get_string(const void *fdt, int stroffset, int *lenp) uint32_t strsize = fdt_size_dt_strings(fdt); const char *s; const char *end; + int err; + + /* off_dt_strings/size_dt_strings are attacker-influenceable header + * fields; validate the layout against totalsize before forming the + * string-table pointer. */ + err = fdt_check_header(fdt); + if (err != 0) { + if (lenp) + *lenp = err; + return NULL; + } if ((stroffset < 0) || ((uint32_t)stroffset >= strsize)) { if (lenp) diff --git a/tools/unit-tests/unit-fdt.c b/tools/unit-tests/unit-fdt.c index 951ed369..516eca3f 100644 --- a/tools/unit-tests/unit-fdt.c +++ b/tools/unit-tests/unit-fdt.c @@ -43,8 +43,12 @@ START_TEST(test_fdt_get_string_rejects_out_of_range_offset) const char *s; memset(&blob, 0, sizeof(blob)); + fdt_set_totalsize(&blob, sizeof(blob.hdr) + sizeof(blob.strings)); fdt_set_off_dt_strings(&blob, sizeof(blob.hdr)); fdt_set_size_dt_strings(&blob, sizeof(blob.strings)); + fdt_set_magic(&blob, FDT_MAGIC); + fdt_set_version(&blob, 17); + fdt_set_last_comp_version(&blob, 16); memcpy(blob.strings, "chosen", sizeof("chosen")); blob.after[0] = 'X'; blob.after[1] = '\0'; @@ -66,8 +70,12 @@ START_TEST(test_fdt_get_string_returns_string_with_valid_offset) const char *s; memset(&blob, 0, sizeof(blob)); + fdt_set_totalsize(&blob, sizeof(blob.hdr) + sizeof(blob.strings)); fdt_set_off_dt_strings(&blob, sizeof(blob.hdr)); fdt_set_size_dt_strings(&blob, sizeof(blob.strings)); + fdt_set_magic(&blob, FDT_MAGIC); + fdt_set_version(&blob, 17); + fdt_set_last_comp_version(&blob, 16); memcpy(blob.strings, "serial\0console\0", 15); s = fdt_get_string(&blob, 7, &len); @@ -218,6 +226,7 @@ static void build_compat_fdt(uint8_t *buf, size_t size, memset(buf, 0, size); hdr = (struct fdt_header *)buf; + hdr->magic = fdt32_to_cpu(FDT_MAGIC); fdt_set_totalsize(hdr, 0x100); fdt_set_off_dt_struct(hdr, struct_off); fdt_set_off_dt_strings(hdr, strings_off); @@ -296,6 +305,41 @@ START_TEST(test_fdt_compatible_prefix_entry_no_match) off = fdt_node_offset_by_compatible(buf, -1, "abc"); ck_assert_int_lt(off, 0); } +END_TEST + +/* A finalized DTB whose string table lies past the declared end of + * the blob must be rejected: before the fix fdt_check_header() + * validated only magic and version, and fdt_get_string() formed the + * string-table pointer from the unvalidated header fields. */ +START_TEST(test_fdt_check_header_rejects_unbounded_string_area) +{ + static uint8_t buf[0x100]; + int len = 0; + const char *s; + + build_compat_fdt(buf, sizeof(buf), (const uint8_t *)"abc\0", 4); + /* push the string table past the declared end of the blob */ + fdt_set_off_dt_strings((struct fdt_header *)buf, 0x100); + + ck_assert_int_eq(fdt_check_header(buf), -FDT_ERR_BADSTRUCTURE); + + s = fdt_get_string(buf, 0, &len); + ck_assert_ptr_null(s); + ck_assert_int_lt(len, 0); +} +END_TEST + +/* The structure block must not overlap the string table. */ +START_TEST(test_fdt_check_header_rejects_overlapping_areas) +{ + static uint8_t buf[0x100]; + + build_compat_fdt(buf, sizeof(buf), (const uint8_t *)"abc\0", 4); + fdt_set_size_dt_struct((struct fdt_header *)buf, 0x100); + + ck_assert_int_eq(fdt_check_header(buf), -FDT_ERR_BADSTRUCTURE); +} +END_TEST static Suite *fdt_suite(void) { @@ -313,6 +357,8 @@ static Suite *fdt_suite(void) tcase_add_test(tc, test_fdt_compatible_terminated_exact_len_match); tcase_add_test(tc, test_fdt_compatible_multi_string_list_match); tcase_add_test(tc, test_fdt_compatible_prefix_entry_no_match); + tcase_add_test(tc, test_fdt_check_header_rejects_unbounded_string_area); + tcase_add_test(tc, test_fdt_check_header_rejects_overlapping_areas); suite_add_tcase(s, tc); tcase_set_timeout(tc_dos, 5); diff --git a/tools/unit-tests/unit-t10xx-dts-memac.c b/tools/unit-tests/unit-t10xx-dts-memac.c index f4db6232..cfb86b6e 100644 --- a/tools/unit-tests/unit-t10xx-dts-memac.c +++ b/tools/unit-tests/unit-t10xx-dts-memac.c @@ -261,7 +261,9 @@ static void dtb_finalize(struct dtb *d) hdr->totalsize = cpu_to_fdt32(0x2000); hdr->off_dt_struct = cpu_to_fdt32(d->struct_off); hdr->off_dt_strings = cpu_to_fdt32(d->strings_off); - hdr->off_mem_rsvmap = cpu_to_fdt32(d->strings_off); /* no reservations */ + hdr->off_mem_rsvmap = cpu_to_fdt32(0x28); + /* the 8 bytes after the 40-byte header are zero: an empty + * reservation list at the canonical spot */ hdr->version = cpu_to_fdt32(17); hdr->last_comp_version = cpu_to_fdt32(16); hdr->boot_cpuid_phys = cpu_to_fdt32(0);