Fix CVE-2018-18440 and CVE-2018-18439

PATCH Superseded Simon Goldschmidt 3 participants 20 messages Dec 9, 2018 – Dec 14, 2018

Subsystems (3)

Networking VFS Library code

lore

Share & tools

lore

Simon Goldschmidt

author

2018-12-09 20:45 UTC #0 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.

The memory reservation code is reused from bootm/image.

Changes in v4:
- added tests for lib/lmb.c
- fixed bug in lmb.c when ram is at the end of 32-bit address range
- fixed a bug in lmb_alloc_addr when resulting reserved rangesa get combined

Changes in v4:
- fixed invalid 'if' statement without braces in boot_fdt_reserve_region
- removed patch 7 ("net: remove CONFIG_MCAST_TFTP), adapted patch 8

Changes in v3:
- No patch changes, but needed to resend since patman added too many cc addresses that gmail seemed to detect as spam :-(

Changes in v2:
- added code to reserve devicetree reserved-memory in lmb
- added tftp fixes (patches 7 and 8)
- fixed a bug in new function lmb_alloc_addr

Simon Goldschmidt (9):
test: add test for lib/lmb.c
lmb: fix allocation at end of address range
lib: lmb: reserving overlapping regions should fail
fdt: parse "reserved-memory" for memory reservation
lib: lmb: extend lmb for checks at load time
fs: prevent overwriting reserved memory
bootm: use new common function lmb_init_and_reserve
lmb: remove unused extern declaration
tftp: prevent overwriting reserved memory

 common/bootm.c     |   8 +-
 common/image-fdt.c |  53 ++++-
 fs/fs.c            |  56 ++++-
 include/lmb.h      |   7 +-
 lib/lmb.c          |  98 ++++++--
 net/tftp.c         |  66 +++++-
 test/lib/Makefile  |   1 +
 test/lib/lmb.c     | 561 +++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 805 insertions(+), 45 deletions(-)
 create mode 100644 test/lib/lmb.c

Signature · 1 line

2.17.1

Simon Goldschmidt

author

2018-12-09 20:45 UTC #1 lore raw

PATCH v5 1/9 test: add test for lib/lmb.c

Add basic tests for the lmb memory allocation code used to reserve and allocate memory during boot.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5:
- this patch is new in v5

 test/lib/Makefile |   1 +
 test/lib/lmb.c    | 297 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 298 insertions(+)
 create mode 100644 test/lib/lmb.c

test/lib/Makefile +1 -0

index ea68fae566..5a636aac74 100644
@@ -3,3 +3,4 @@
 # (C) Copyright 2018
 # Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
 obj-y += hexdump.o
+obj-y += lmb.o

test/lib/lmb.c new file +297 -0

297 lines not on this page · raw message

Signature · 1 line

2.17.1

Simon Glass

2018-12-11 01:05 UTC #2 lore raw

PATCH v5 1/9 test: add test for lib/lmb.c

Quoted 7 lines · Simon Goldschmidt

Add basic tests for the lmb memory allocation code used to reserve and allocate memory during boot.
Signed-off-by Simon Goldschmidt <[email protected]>
---

Quoted 7 lines

Changes in v5:
- this patch is new in v5

 test/lib/Makefile |   1 +
 test/lib/lmb.c    | 297 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 298 insertions(+)
 create mode 100644 test/lib/lmb.c

Reviewed-by Simon Glass <[email protected]>

Simon Goldschmidt

author

2018-12-09 20:45 UTC #3 lore raw

PATCH v5 2/9 lmb: fix allocation at end of address range

In reply to Simon Goldschmidt

The lmb code fails if base + size of RAM overflows to zero.

Fix this by calculating end as 'base + size - 1' instead of 'base + size' where appropriate.

Added tests to assert this is fixed.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5:
- this patch is new in v5

 lib/lmb.c      | 29 ++++++++++++-----------------
 test/lib/lmb.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 38 insertions(+), 20 deletions(-)

lib/lmb.c +12 ~~-17~~

index 1705417348..6d3dcf4e09 100644
@@ -43,7 +43,10 @@ void lmb_dump_all(struct lmb *lmb)
 static long lmb_addrs_overlap(phys_addr_t base1,
 		phys_size_t size1, phys_addr_t base2, phys_size_t size2)
 {
-	return ((base1 < (base2+size2)) && (base2 < (base1+size1)));
+	const phys_addr_t base1_end = base1 + size1 - 1;
+	const phys_addr_t base2_end = base2 + size2 - 1;
+
+	return ((base1 <= base2_end) && (base2 <= base1_end));
 }
 
 static long lmb_addrs_adjacent(phys_addr_t base1, phys_size_t size1,

@@ -89,18 +92,9 @@ static void lmb_coalesce_regions(struct lmb_region *rgn,
 
 void lmb_init(struct lmb *lmb)
 {
-	/* Create a dummy zero size LMB which will get coalesced away later.
-	 * This simplifies the lmb_add() code below...
-	 */
-	lmb->memory.region[0].base = 0;
-	lmb->memory.region[0].size = 0;
-	lmb->memory.cnt = 1;
+	lmb->memory.cnt = 0;
 	lmb->memory.size = 0;
-
-	/* Ditto. */
-	lmb->reserved.region[0].base = 0;
-	lmb->reserved.region[0].size = 0;
-	lmb->reserved.cnt = 1;
+	lmb->reserved.cnt = 0;
 	lmb->reserved.size = 0;
 }

@@ -110,9 +104,10 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	unsigned long coalesced = 0;
 	long adjacent, i;
 
-	if ((rgn->cnt == 1) && (rgn->region[0].size == 0)) {
+	if (rgn->cnt == 0) {
 		rgn->region[0].base = base;
 		rgn->region[0].size = size;
+		rgn->cnt = 1;
 		return 0;
 	}

@@ -183,7 +178,7 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 {
 	struct lmb_region *rgn = &(lmb->reserved);
 	phys_addr_t rgnbegin, rgnend;
-	phys_addr_t end = base + size;
+	phys_addr_t end = base + size - 1;
 	int i;
 
 	rgnbegin = rgnend = 0; /* supress gcc warnings */

@@ -191,7 +186,7 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 	/* Find the region where (base, size) belongs to */
 	for (i=0; i < rgn->cnt; i++) {
 		rgnbegin = rgn->region[i].base;
-		rgnend = rgnbegin + rgn->region[i].size;
+		rgnend = rgnbegin + rgn->region[i].size - 1;
 
 		if ((rgnbegin <= base) && (end <= rgnend))
 			break;

@@ -209,7 +204,7 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 
 	/* Check to see if region is matching@the front */
 	if (rgnbegin == base) {
-		rgn->region[i].base = end;
+		rgn->region[i].base = end + 1;
 		rgn->region[i].size -= size;
 		return 0;
 	}

@@ -225,7 +220,7 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 	 * beginging of the hole and add the region after hole.
 	 */
 	rgn->region[i].size = base - rgn->region[i].base;
-	return lmb_add_region(rgn, end, rgnend - end);
+	return lmb_add_region(rgn, end + 1, rgnend - end);
 }
 
 long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size)

test/lib/lmb.c +26 -3

index dd7ba14b34..fb7ca45ef1 100644
@@ -146,8 +146,15 @@ static int test_multi_alloc_512mb(struct unit_test_state *uts,
 /* Create a memory region with one reserved region and allocate */
 static int lib_test_lmb_simple(struct unit_test_state *uts)
 {
+	int ret;
+
 	/* simulate 512 MiB RAM beginning at 1GiB */
-	return test_multi_alloc_512mb(uts, 0x40000000);
+	ret = test_multi_alloc_512mb(uts, 0x40000000);
+	if (ret)
+		return ret;
+
+	/* simulate 512 MiB RAM beginning at 1.5GiB */
+	return test_multi_alloc_512mb(uts, 0xE0000000);
 }
 
 DM_TEST(lib_test_lmb_simple, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);

@@ -206,7 +213,15 @@ static int test_bigblock(struct unit_test_state *uts, const phys_addr_t ram)
 
 static int lib_test_lmb_big(struct unit_test_state *uts)
 {
-	return test_bigblock(uts, 0x40000000);
+	int ret;
+
+	/* simulate 512 MiB RAM beginning at 1GiB */
+	ret = test_bigblock(uts, 0x40000000);
+	if (ret)
+		return ret;
+
+	/* simulate 512 MiB RAM beginning at 1.5GiB */
+	return test_bigblock(uts, 0xE0000000);
 }
 
 DM_TEST(lib_test_lmb_big, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);

@@ -247,7 +262,15 @@ static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram)
 
 static int lib_test_lmb_noreserved(struct unit_test_state *uts)
 {
-	return test_noreserved(uts, 0x40000000);
+	int ret;
+
+	/* simulate 512 MiB RAM beginning at 1GiB */
+	ret = test_noreserved(uts, 0x40000000);
+	if (ret)
+		return ret;
+
+	/* simulate 512 MiB RAM beginning at 1.5GiB */
+	return test_noreserved(uts, 0xE0000000);
 }
 
 DM_TEST(lib_test_lmb_noreserved, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);

Signature · 1 line

2.17.1

Simon Goldschmidt

author

2018-12-09 20:45 UTC #4 lore raw

PATCH v5 3/9 lib: lmb: reserving overlapping regions should fail

In reply to Simon Goldschmidt

lmb_add_region handles overlapping regions wrong: instead of merging or rejecting to add a new reserved region that overlaps an existing one, it just adds the new region.

Since internally the same function is used for lmb_alloc, change lmb_add_region to reject overlapping regions.

Add test to assert this.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5:
- added a test for this bug

Changes in v4: None
Changes in v2: None

 lib/lmb.c      |  3 +++
 test/lib/lmb.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

lib/lmb.c +3 -0

index 6d3dcf4e09..62a306c5b9 100644
@@ -131,6 +131,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
+		} else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
+			/* regions overlap */
+			return -1;
 		}
 	}

test/lib/lmb.c +39 -0

index fb7ca45ef1..c6823b3f3d 100644
@@ -318,3 +318,42 @@ static int lib_test_lmb_at_0(struct unit_test_state *uts)
 }
 
 DM_TEST(lib_test_lmb_at_0, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Check that calling lmb_reserve with overlapping regions fails. */
+static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts)
+{
+	const phys_addr_t ram = 0x40000000;
+	const phys_size_t ram_size = 0x20000000;
+	struct lmb lmb;
+	long ret;
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	ret = lmb_reserve(&lmb, 0x40010000, 0x10000);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+	/* allocate overlapping region should fail */
+	ret = lmb_reserve(&lmb, 0x40011000, 0x10000);
+	ut_asserteq(ret, -1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+	/* allocate 3nd region */
+	ret = lmb_reserve(&lmb, 0x40030000, 0x10000);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40010000, 0x10000,
+		   0x40030000, 0x10000, 0, 0);
+	/* allocate 2nd region */
+	ret = lmb_reserve(&lmb, 0x40020000, 0x10000);
+	ut_assert(ret >= 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x30000,
+		   0, 0, 0, 0);
+
+	return 0;
+}
+
+DM_TEST(lib_test_lmb_overlapping_reserve,
+	DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);

Signature · 1 line

2.17.1

Simon Goldschmidt

author

2018-12-09 20:45 UTC #5 lore raw

PATCH v5 4/9 fdt: parse "reserved-memory" for memory reservation

In reply to Simon Goldschmidt

boot_fdt_add_mem_rsv_regions() adds reserved memory sections to an lmb struct. Currently, it only parses regions described by /memreserve/ entries.

Extend this to the more commonly used scheme of the "reserved-memory" node.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5: None
Changes in v4:
- fixed invalid 'if' statement without braces in boot_fdt_reserve_region

Changes in v2:
- this patch is new in v2

 common/image-fdt.c | 53 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 8 deletions(-)

common/image-fdt.c +45 -8

index 95748f0ae1..5c0d6db3fe 100644
@@ -10,6 +10,7 @@
 
 #include <common.h>
 #include <fdt_support.h>
+#include <fdtdec.h>
 #include <errno.h>
 #include <image.h>
 #include <linux/libfdt.h>

@@ -67,30 +68,66 @@ static const image_header_t *image_get_fdt(ulong fdt_addr)
 }
 #endif
 
+static void boot_fdt_reserve_region(struct lmb *lmb, uint64_t addr,
+				    uint64_t size)
+{
+	int ret;
+
+	ret = lmb_reserve(lmb, addr, size);
+	if (!ret) {
+		debug("   reserving fdt memory region: addr=%llx size=%llx\n",
+		      (unsigned long long)addr, (unsigned long long)size);
+	} else {
+		puts("ERROR: reserving fdt memory region failed ");
+		printf("(addr=%llx size=%llx)\n",
+		       (unsigned long long)addr, (unsigned long long)size);
+	}
+}
+
 /**
- * boot_fdt_add_mem_rsv_regions - Mark the memreserve sections as unusable
+ * boot_fdt_add_mem_rsv_regions - Mark the memreserve and reserved-memory
+ * sections as unusable
  * @lmb: pointer to lmb handle, will be used for memory mgmt
  * @fdt_blob: pointer to fdt blob base address
  *
- * Adds the memreserve regions in the dtb to the lmb block.  Adding the
- * memreserve regions prevents u-boot from using them to store the initrd
- * or the fdt blob.
+ * Adds the and reserved-memorymemreserve regions in the dtb to the lmb block.
+ * Adding the memreserve regions prevents u-boot from using them to store the
+ * initrd or the fdt blob.
  */
 void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 {
 	uint64_t addr, size;
-	int i, total;
+	int i, total, ret;
+	int nodeoffset, subnode;
+	struct fdt_resource res;
 
 	if (fdt_check_header(fdt_blob) != 0)
 		return;
 
+	/* process memreserve sections */
 	total = fdt_num_mem_rsv(fdt_blob);
 	for (i = 0; i < total; i++) {
 		if (fdt_get_mem_rsv(fdt_blob, i, &addr, &size) != 0)
 			continue;
-		printf("   reserving fdt memory region: addr=%llx size=%llx\n",
-		       (unsigned long long)addr, (unsigned long long)size);
-		lmb_reserve(lmb, addr, size);
+		boot_fdt_reserve_region(lmb, addr, size);
+	}
+
+	/* process reserved-memory */
+	nodeoffset = fdt_subnode_offset(fdt_blob, 0, "reserved-memory");
+	if (nodeoffset >= 0) {
+		subnode = fdt_first_subnode(fdt_blob, nodeoffset);
+		while (subnode >= 0) {
+			/* check if this subnode has a reg property */
+			ret = fdt_get_resource(fdt_blob, subnode, "reg", 0,
+					       &res);
+			if (!ret) {
+				addr = res.start;
+				size = res.end - res.start + 1;
+				boot_fdt_reserve_region(lmb, addr, size);
+			}
+
+			subnode = fdt_next_subnode(fdt_blob, subnode);
+		}
 	}
 }

Signature · 1 line

2.17.1

Simon Glass

2018-12-11 01:05 UTC #6 lore raw

PATCH v5 4/9 fdt: parse "reserved-memory" for memory reservation

Quoted 15 lines · Simon Goldschmidt

boot_fdt_add_mem_rsv_regions() adds reserved memory sections to an lmb struct. Currently, it only parses regions described by /memreserve/ entries.
Extend this to the more commonly used scheme of the "reserved-memory" node.
Signed-off-by Simon Goldschmidt <[email protected]>
---
Changes in v5: None
Changes in v4:
- fixed invalid 'if' statement without braces in boot_fdt_reserve_region

Quoted 6 lines

Changes in v2:
- this patch is new in v2

 common/image-fdt.c | 53 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 8 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Simon Goldschmidt

author

2018-12-09 20:45 UTC #7 lore raw

PATCH v5 5/9 lib: lmb: extend lmb for checks at load time

In reply to Simon Goldschmidt

This adds two new functions, lmb_alloc_addr and
lmb_get_unreserved_size.

lmb_alloc_addr behaves like lmb_alloc, but it tries to allocate a pre-specified address range. Unlike lmb_reserve, this address range must be inside one of the memory ranges that has been set up with lmb_add.

lmb_get_unreserved_size returns the number of bytes that can be used up to the next reserved region or the end of valid ram. This can be 0 if the address passed is reserved.

Added test for these new functions.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5:
- fixed lmb_alloc_addr when resulting reserved ranges get combined
- added test for these new functions

Changes in v4: None
Changes in v2:
- added lmb_get_unreserved_size() for tftp

 include/lmb.h  |   3 +
 lib/lmb.c      |  53 +++++++++++++
 test/lib/lmb.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)

include/lmb.h +3 -0

index f04d058093..7d7e2a78dc 100644
@@ -38,6 +38,9 @@ extern phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align
 			    phys_addr_t max_addr);
 extern phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align,
 			      phys_addr_t max_addr);
+extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
+				  phys_size_t size);
+extern phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr);
 extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
 extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);

lib/lmb.c +53 -0

59 lines not on this page · raw message

test/lib/lmb.c +202 -0

205 lines not on this page · raw message

Signature · 1 line

2.17.1

Simon Goldschmidt

author

2018-12-09 20:45 UTC #8 lore raw

PATCH v5 6/9 fs: prevent overwriting reserved memory

In reply to Simon Goldschmidt

This fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by using lmb to check the load size of a file against reserved memory addresses.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5: None
Changes in v4: None
Changes in v2: None

 fs/fs.c       | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/lmb.h |  2 ++
 lib/lmb.c     | 13 ++++++++++++
 3 files changed, 68 insertions(+), 3 deletions(-)

fs/fs.c +53 -3

index cb265174e2..400aa921a7 100644
@@ -429,13 +429,57 @@ int fs_size(const char *filename, loff_t *size)
 	return ret;
 }
 
-int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
-	    loff_t *actread)
+#ifdef CONFIG_LMB
+/* Check if a file may be read to the given address */
+static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset,
+			     loff_t len, struct fstype_info *info)
+{
+	struct lmb lmb;
+	int ret;
+	loff_t size;
+	loff_t read_len;
+
+	/* get the actual size of the file */
+	ret = info->size(filename, &size);
+	if (ret)
+		return ret;
+	if (offset >= size) {
+		/* offset >= EOF, no bytes will be written */
+		return 0;
+	}
+	read_len = size - offset;
+
+	/* limit to 'len' if it is smaller */
+	if (len && len < read_len)
+		read_len = len;
+
+	lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start,
+			     gd->bd->bi_dram[0].size, (void *)gd->fdt_blob);
+	lmb_dump_all(&lmb);
+
+	if (lmb_alloc_addr(&lmb, addr, read_len) == addr)
+		return 0;
+
+	printf("** Reading file would overwrite reserved memory **\n");
+	return -1;
+}
+#endif
+
+static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
+		    int do_lmb_check, loff_t *actread)
 {
 	struct fstype_info *info = fs_get_info(fs_type);
 	void *buf;
 	int ret;
 
+#ifdef CONFIG_LMB
+	if (do_lmb_check) {
+		ret = fs_read_lmb_check(filename, addr, offset, len, info);
+		if (ret)
+			return ret;
+	}
+#endif
+
 	/*
 	 * We don't actually know how many bytes are being read, since len==0
 	 * means read the whole file.

@@ -452,6 +496,12 @@ int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
 	return ret;
 }
 
+int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
+	    loff_t *actread)
+{
+	return _fs_read(filename, addr, offset, len, 0, actread);
+}
+
 int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
 	     loff_t *actwrite)
 {

@@ -622,7 +672,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		pos = 0;
 
 	time = get_timer(0);
-	ret = fs_read(filename, addr, pos, bytes, &len_read);
+	ret = _fs_read(filename, addr, pos, bytes, 1, &len_read);
 	time = get_timer(time);
 	if (ret < 0)
 		return 1;

include/lmb.h +2 -0

index 7d7e2a78dc..62da85e716 100644
@@ -31,6 +31,8 @@ struct lmb {
 extern struct lmb lmb;
 
 extern void lmb_init(struct lmb *lmb);
+extern void lmb_init_and_reserve(struct lmb *lmb, phys_addr_t base,
+				 phys_size_t size, void *fdt_blob);
 extern long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size);
 extern long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size);
 extern phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align);

lib/lmb.c +13 -0

index 04fe53f355..ba680f883e 100644
@@ -98,6 +98,19 @@ void lmb_init(struct lmb *lmb)
 	lmb->reserved.size = 0;
 }
 
+/* Initialize the struct, add memory and call arch/board reserve functions */
+void lmb_init_and_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size,
+			  void *fdt_blob)
+{
+	lmb_init(lmb);
+	lmb_add(lmb, base, size);
+	arch_lmb_reserve(lmb);
+	board_lmb_reserve(lmb);
+
+	if (IMAGE_ENABLE_OF_LIBFDT)
+		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
+}
+
 /* This routine called with relocation disabled. */
 static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t size)
 {

Signature · 1 line

2.17.1

Simon Glass

2018-12-11 01:07 UTC #9 lore raw

PATCH v5 6/9 fs: prevent overwriting reserved memory

Quoted 8 lines · Simon Goldschmidt

This fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by using lmb to check the load size of a file against reserved memory addresses.
Signed-off-by Simon Goldschmidt <[email protected]>
---

Quoted 9 lines

Changes in v5: None
Changes in v4: None
Changes in v2: None

 fs/fs.c       | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/lmb.h |  2 ++
 lib/lmb.c     | 13 ++++++++++++
 3 files changed, 68 insertions(+), 3 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Simon Goldschmidt

author

2018-12-09 20:45 UTC #10 lore raw

PATCH v5 7/9 bootm: use new common function lmb_init_and_reserve

In reply to Simon Goldschmidt

This reduces duplicate code only.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5: None
Changes in v4: None
Changes in v2: None

 common/bootm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

common/bootm.c +2 -6

index 8bf84ebcb7..31e4f0f794 100644
@@ -56,15 +56,11 @@ static void boot_start_lmb(bootm_headers_t *images)
 	ulong		mem_start;
 	phys_size_t	mem_size;
 
-	lmb_init(&images->lmb);
-
 	mem_start = env_get_bootm_low();
 	mem_size = env_get_bootm_size();
 
-	lmb_add(&images->lmb, (phys_addr_t)mem_start, mem_size);
-
-	arch_lmb_reserve(&images->lmb);
-	board_lmb_reserve(&images->lmb);
+	lmb_init_and_reserve(&images->lmb, (phys_addr_t)mem_start, mem_size,
+			     NULL);
 }
 #else
 #define lmb_reserve(lmb, base, size)

Signature · 1 line

2.17.1

Simon Glass

2018-12-11 01:07 UTC #11 lore raw

PATCH v5 7/9 bootm: use new common function lmb_init_and_reserve

Quoted 4 lines · Simon Goldschmidt

This reduces duplicate code only.
Signed-off-by Simon Goldschmidt <[email protected]>

Quoted 9 lines

---
Changes in v5: None
Changes in v4: None
Changes in v2: None
 common/bootm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Simon Goldschmidt

author

2018-12-09 20:45 UTC #12 lore raw

PATCH v5 8/9 lmb: remove unused extern declaration

In reply to Simon Goldschmidt

lmb.h includes an extern declaration of "struct lmb lmb;" which is not used anywhere, so remove it.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5: None
Changes in v4: None
Changes in v2:
- this patch is new in v2

 include/lmb.h | 2 --
 1 file changed, 2 deletions(-)

include/lmb.h +0 -2

index 62da85e716..1bb003e35e 100644
@@ -28,8 +28,6 @@ struct lmb {
 	struct lmb_region reserved;
 };
 
-extern struct lmb lmb;
-
 extern void lmb_init(struct lmb *lmb);
 extern void lmb_init_and_reserve(struct lmb *lmb, phys_addr_t base,
 				 phys_size_t size, void *fdt_blob);

Signature · 1 line

2.17.1

Simon Goldschmidt

author

2018-12-09 20:45 UTC #13 lore raw

PATCH v5 9/9 tftp: prevent overwriting reserved memory

In reply to Simon Goldschmidt

This fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by using lmb to check for a valid range to store received blocks.

Signed-off-by Simon Goldschmidt <[email protected]>

---

Changes in v5: None
Changes in v4:
- this was patch 8, is now patch 7
- lines changed because v3 patch 7 got removed and MCAST_TFTP still exists

Changes in v2:
- this patch is new in v2

 net/tftp.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 9 deletions(-)

net/tftp.c +57 -9

index 68ffd81414..d31364166e 100644
@@ -17,6 +17,8 @@
 #include <flash.h>
 #endif
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /* Well known TFTP port # */
 #define WELL_KNOWN_PORT	69
 /* Millisecs to timeout for lost pkt */

@@ -81,6 +83,8 @@ static ulong	tftp_block_wrap;
 /* memory offset due to wrapping */
 static ulong	tftp_block_wrap_offset;
 static int	tftp_state;
+static ulong	tftp_load_addr;
+static ulong	tftp_load_size;
 #ifdef CONFIG_TFTP_TSIZE
 /* The file size reported by the server */
 static int	tftp_tsize;

@@ -164,10 +168,11 @@ static void mcast_cleanup(void)
 
 #endif	/* CONFIG_MCAST_TFTP */
 
-static inline void store_block(int block, uchar *src, unsigned len)
+static inline int store_block(int block, uchar *src, unsigned int len)
 {
 	ulong offset = block * tftp_block_size + tftp_block_wrap_offset;
 	ulong newsize = offset + len;
+	ulong store_addr = tftp_load_addr + offset;
 #ifdef CONFIG_SYS_DIRECT_FLASH_TFTP
 	int i, rc = 0;

@@ -175,24 +180,30 @@ static inline void store_block(int block, uchar *src, unsigned len)
 		/* start address in flash? */
 		if (flash_info[i].flash_id == FLASH_UNKNOWN)
 			continue;
-		if (load_addr + offset >= flash_info[i].start[0]) {
+		if (store_addr >= flash_info[i].start[0]) {
 			rc = 1;
 			break;
 		}
 	}
 
 	if (rc) { /* Flash is destination for this packet */
-		rc = flash_write((char *)src, (ulong)(load_addr+offset), len);
+		rc = flash_write((char *)src, store_addr, len);
 		if (rc) {
 			flash_perror(rc);
-			net_set_state(NETLOOP_FAIL);
-			return;
+			return rc;
 		}
 	} else
 #endif /* CONFIG_SYS_DIRECT_FLASH_TFTP */
 	{
-		void *ptr = map_sysmem(load_addr + offset, len);
+		void *ptr;
 
+		if (store_addr < tftp_load_addr ||
+		    store_addr + len > tftp_load_addr + tftp_load_size) {
+			puts("\nTFTP error: ");
+			puts("trying to overwrite reserved memory...\n");
+			return -1;
+		}
+		ptr = map_sysmem(store_addr, len);
 		memcpy(ptr, src, len);
 		unmap_sysmem(ptr);
 	}

@@ -203,6 +214,8 @@ static inline void store_block(int block, uchar *src, unsigned len)
 
 	if (net_boot_file_size < newsize)
 		net_boot_file_size = newsize;
+
+	return 0;
 }
 
 /* Clear our state ready for a new transfer */

@@ -606,7 +619,11 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 		timeout_count_max = tftp_timeout_count_max;
 		net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
 
-		store_block(tftp_cur_block - 1, pkt + 2, len);
+		if (store_block(tftp_cur_block - 1, pkt + 2, len)) {
+			eth_halt();
+			net_set_state(NETLOOP_FAIL);
+			break;
+		}
 
 		/*
 		 *	Acknowledge the block just received, which will prompt

@@ -695,6 +712,24 @@ static void tftp_timeout_handler(void)
 	}
 }
 
+/* Initialize tftp_load_addr and tftp_load_size from load_addr and lmb */
+static int tftp_init_load_addr(void)
+{
+	struct lmb lmb;
+	phys_size_t max_size;
+
+	tftp_load_addr = load_addr;
+
+	lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start,
+			     gd->bd->bi_dram[0].size, (void *)gd->fdt_blob);
+
+	max_size = lmb_get_unreserved_size(&lmb, tftp_load_addr);
+	if (!max_size)
+		return -1;
+
+	tftp_load_size = max_size;
+	return 0;
+}
 
 void tftp_start(enum proto_t protocol)
 {

@@ -791,7 +826,14 @@ void tftp_start(enum proto_t protocol)
 	} else
 #endif
 	{
-		printf("Load address: 0x%lx\n", load_addr);
+		if (tftp_init_load_addr()) {
+			eth_halt();
+			net_set_state(NETLOOP_FAIL);
+			puts("\nTFTP error: ");
+			puts("trying to overwrite reserved memory...\n");
+			return;
+		}
+		printf("Load address: 0x%lx\n", tftp_load_addr);
 		puts("Loading: *\b");
 		tftp_state = STATE_SEND_RRQ;
 #ifdef CONFIG_CMD_BOOTEFI

@@ -842,9 +884,15 @@ void tftp_start_server(void)
 {
 	tftp_filename[0] = 0;
 
+	if (tftp_init_load_addr()) {
+		eth_halt();
+		net_set_state(NETLOOP_FAIL);
+		puts("\nTFTP error: trying to overwrite reserved memory...\n");
+		return;
+	}
 	printf("Using %s device\n", eth_get_name());
 	printf("Listening for TFTP transfer on %pI4\n", &net_ip);
-	printf("Load address: 0x%lx\n", load_addr);
+	printf("Load address: 0x%lx\n", tftp_load_addr);
 
 	puts("Loading: *\b");

Signature · 1 line

2.17.1

Tom Rini

2018-12-11 13:31 UTC #14 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

In reply to Simon Goldschmidt

Quoted 10 lines · Simon Goldschmidt

This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.
The memory reservation code is reused from bootm/image.

So, thanks for doing all of this. I'm sorry to dump a few problems on you now however. First, we have a lot of fail to builds:
https://travis-ci.org/trini/u-boot/builds/466333708

Second, giving this a run-time test on Raspberry Pi 3 (aarch64 mode) and
trying to boot a regular Linux distro (this example is OpenEmbedded
based but generic issue, boot.scr just loads Image to $loadaddr and
booti's):
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
389 bytes read in 2 ms (189.5 KiB/s)
## Executing script at 02000000
13298176 bytes read in 701 ms (18.1 MiB/s)
## Flattened Device Tree blob at 2effb500
Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device TreeFDT creation failed! hanging...### ERROR ### Please RESET the board ###

Switching the board to using bootm_size rather than initrd_high/fdt_high=0xffffffff does resolve the issue and I can boot, but it's still a case we need to fix. Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181211/eb21f120/attachment.sig>

Simon Goldschmidt

author

2018-12-11 15:19 UTC #15 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

Hi Tom,

[truncated the CC list a bit since I got "too many recipients" errors last time]

Am 11.12.2018 um 14:31 schrieb Tom Rini:

Quoted 11 lines · Simon Goldschmidt

Quoted 10 lines
This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.

Quoted 5 lines

Quoted 1 line
The memory reservation code is reused from bootm/image.
So, thanks for doing all of this. I'm sorry to dump a few problems on you now however. First, we have a lot of fail to builds:
https://travis-ci.org/trini/u-boot/builds/466333708

Ok, I'll check those. At first sight, the build errors seem to be identical in that 'fdt_get_resource' is missing. I'll check that config option.

Quoted 13 lines

Second, giving this a run-time test on Raspberry Pi 3 (aarch64 mode) and trying to boot a regular Linux distro (this example is OpenEmbedded based but generic issue, boot.scr just loads Image to $loadaddr and booti's):
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
389 bytes read in 2 ms (189.5 KiB/s)
## Executing script at 02000000
13298176 bytes read in 701 ms (18.1 MiB/s)
## Flattened Device Tree blob at 2effb500

Quoted 7 lines

    Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device TreeFDT creation failed! hanging...### ERROR ### Please RESET the board ###
Switching the board to using bootm_size rather than initrd_high/fdt_high=0xffffffff does resolve the issue and I can boot, but it's still a case we need to fix. Thanks!

Thanks for testing! Of course it's a case we need to fix! Would it be possible for you to do this run-time test again with the attached patch that enables DEBUG in lmb.c and dumps 'lmb' contents in the error case shown above?

Regards,
Simon

-------------- next part --------------

Tom Rini

2018-12-11 20:10 UTC #16 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

Quoted 43 lines · Simon Goldschmidt

Hi Tom,
[truncated the CC list a bit since I got "too many recipients" errors last time]
Am 11.12.2018 um 14:31 schrieb Tom Rini:
Quoted 11 lines · Simon Goldschmidt
Quoted 10 lines
This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.
Quoted 5 lines
Quoted 1 line
The memory reservation code is reused from bootm/image.
So, thanks for doing all of this. I'm sorry to dump a few problems on you now however. First, we have a lot of fail to builds:
https://travis-ci.org/trini/u-boot/builds/466333708
Ok, I'll check those. At first sight, the build errors seem to be identical in that 'fdt_get_resource' is missing. I'll check that config option.
Quoted 17 lines
Second, giving this a run-time test on Raspberry Pi 3 (aarch64 mode) and
trying to boot a regular Linux distro (this example is OpenEmbedded
based but generic issue, boot.scr just loads Image to $loadaddr and
booti's):
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
389 bytes read in 2 ms (189.5 KiB/s)
## Executing script at 02000000
13298176 bytes read in 701 ms (18.1 MiB/s)
## Flattened Device Tree blob at 2effb500
Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device TreeFDT creation failed! hanging...### ERROR ### Please RESET the board ###

Quoted 8 lines

Quoted 3 lines
Switching the board to using bootm_size rather than initrd_high/fdt_high=0xffffffff does resolve the issue and I can boot, but it's still a case we need to fix. Thanks!
Thanks for testing! Of course it's a case we need to fix! Would it be possible for you to do this run-time test again with the attached patch that enables DEBUG in lmb.c and dumps 'lmb' contents in the error case shown above?

Here. Note that I'm sure you can replicate this anywhere by setting initrd_high / fdt_high to 0xffffffff.
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
lmb_dump_all:

    memory.cnt		   = 0x1
    memory.size		   = 0x0
    memory.reg[0x0].base   = 0x0
		   .size   = 0x3b400000

    reserved.cnt	   = 0x2
    reserved.size	   = 0x0
    reserved.reg[0x0].base = 0x0
		     .size = 0x1000
    reserved.reg[0x1].base = 0x3af46e50
		     .size = 0x4b91b0
389 bytes read in 28 ms (12.7 KiB/s)
## Executing script at 02000000
lmb_dump_all:
    memory.cnt		   = 0x1
    memory.size		   = 0x0
    memory.reg[0x0].base   = 0x0
		   .size   = 0x3b400000

    reserved.cnt	   = 0x2
    reserved.size	   = 0x0
    reserved.reg[0x0].base = 0x0
		     .size = 0x1000
    reserved.reg[0x1].base = 0x3af46a20
		     .size = 0x4b95e0

13298176 bytes read in 746 ms (17 MiB/s)
## Flattened Device Tree blob at 2effb500
Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff. Failed using fdt_high value for Device Treelmb_dump_all:

    memory.cnt		   = 0x1
    memory.size		   = 0x0
    memory.reg[0x0].base   = 0x0
		   .size   = 0x3b400000

    reserved.cnt	   = 0x3
    reserved.size	   = 0x0
    reserved.reg[0x0].base = 0x0
		     .size = 0x1000
    reserved.reg[0x1].base = 0x1080000
		     .size = 0xd83000
    reserved.reg[0x2].base = 0x3af46b10
		     .size = 0x4b94f0

FDT creation failed! hanging...### ERROR ### Please RESET the board ###

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181211/bc8dac29/attachment.sig>

Simon Goldschmidt

author

2018-12-11 21:05 UTC #17 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

Am 11.12.2018 um 21:10 schrieb Tom Rini:

Quoted 18 lines · Simon Goldschmidt

Quoted 17 lines
Hi Tom,
[truncated the CC list a bit since I got "too many recipients" errors last time]
Am 11.12.2018 um 14:31 schrieb Tom Rini:
Quoted 11 lines · Simon Goldschmidt
Quoted 10 lines
This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
The memory reservation code is reused from bootm/image.
So, thanks for doing all of this. I'm sorry to dump a few problems on you now however. First, we have a lot of fail to builds:
https://travis-ci.org/trini/u-boot/builds/466333708
Ok, I'll check those. At first sight, the build errors seem to be identical in that 'fdt_get_resource' is missing. I'll check that config option.

The function 'fdt_get_resource' was the only match that I could find to decode register address + size from fdt. However, it resides in 'lib/fdtdec.c' which is only linked for OF_CONTROL. This seems strange as it is a read-access function to a dts and it can be used when booting, too. What would be the way to go here, move this to a different file or compile 'lib/fdtdec.c' depending on CONFIG_FIT or something?

2nd fail seems to be in 'test/py' tests. I'll dig into that, too.

Quoted 22 lines

Quoted 22 lines
Quoted 21 lines
Second, giving this a run-time test on Raspberry Pi 3 (aarch64 mode) and trying to boot a regular Linux distro (this example is OpenEmbedded based but generic issue, boot.scr just loads Image to $loadaddr and booti's):
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
389 bytes read in 2 ms (189.5 KiB/s)
## Executing script at 02000000
13298176 bytes read in 701 ms (18.1 MiB/s)
## Flattened Device Tree blob at 2effb500
    Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device TreeFDT creation failed! hanging...### ERROR ### Please RESET the board ###
Switching the board to using bootm_size rather than initrd_high/fdt_high=0xffffffff does resolve the issue and I can boot, but it's still a case we need to fix. Thanks!

Quoted 7 lines

Quoted 4 lines
Thanks for testing! Of course it's a case we need to fix! Would it be possible for you to do this run-time test again with the attached patch that enables DEBUG in lmb.c and dumps 'lmb' contents in the error case shown above?
Here. Note that I'm sure you can replicate this anywhere by setting initrd_high / fdt_high to 0xffffffff.

OK, thanks for this additional output. I cannot reproduce this in the tests though. I'll have to check this with my hardware probably, but I hope it's not related to 64 bit...

Regards,
Simon

Quoted 51 lines

U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
lmb_dump_all:

     memory.cnt		   = 0x1
     memory.size		   = 0x0
     memory.reg[0x0].base   = 0x0
		   .size   = 0x3b400000

     reserved.cnt	   = 0x2
     reserved.size	   = 0x0
     reserved.reg[0x0].base = 0x0
		     .size = 0x1000
     reserved.reg[0x1].base = 0x3af46e50
		     .size = 0x4b91b0
389 bytes read in 28 ms (12.7 KiB/s)
## Executing script at 02000000
lmb_dump_all:
     memory.cnt		   = 0x1
     memory.size		   = 0x0
     memory.reg[0x0].base   = 0x0
		   .size   = 0x3b400000

     reserved.cnt	   = 0x2
     reserved.size	   = 0x0
     reserved.reg[0x0].base = 0x0
		     .size = 0x1000
     reserved.reg[0x1].base = 0x3af46a20
		     .size = 0x4b95e0
13298176 bytes read in 746 ms (17 MiB/s)
## Flattened Device Tree blob at 2effb500
    Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device Treelmb_dump_all:
     memory.cnt		   = 0x1
     memory.size		   = 0x0
     memory.reg[0x0].base   = 0x0
		   .size   = 0x3b400000

     reserved.cnt	   = 0x3
     reserved.size	   = 0x0
     reserved.reg[0x0].base = 0x0
		     .size = 0x1000
     reserved.reg[0x1].base = 0x1080000
		     .size = 0xd83000
     reserved.reg[0x2].base = 0x3af46b10
		     .size = 0x4b94f0

FDT creation failed! hanging...### ERROR ### Please RESET the board ###

Simon Goldschmidt

author

2018-12-14 13:06 UTC #18 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

Quoted 68 lines · Simon Goldschmidt

Am 11.12.2018 um 21:10 schrieb Tom Rini:
Quoted 18 lines · Simon Goldschmidt
Quoted 17 lines
Hi Tom,
[truncated the CC list a bit since I got "too many recipients" errors last time]
Am 11.12.2018 um 14:31 schrieb Tom Rini:
Quoted 11 lines · Simon Goldschmidt
Quoted 10 lines
This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
The memory reservation code is reused from bootm/image.
So, thanks for doing all of this. I'm sorry to dump a few problems on you now however. First, we have a lot of fail to builds:
https://travis-ci.org/trini/u-boot/builds/466333708
Ok, I'll check those. At first sight, the build errors seem to be identical in that 'fdt_get_resource' is missing. I'll check that config option.
The function 'fdt_get_resource' was the only match that I could find to decode register address + size from fdt. However, it resides in 'lib/fdtdec.c' which is only linked for OF_CONTROL. This seems strange as it is a read-access function to a dts and it can be used when booting, too. What would be the way to go here, move this to a different file or compile 'lib/fdtdec.c' depending on CONFIG_FIT or something?
2nd fail seems to be in 'test/py' tests. I'll dig into that, too.
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
Second, giving this a run-time test on Raspberry Pi 3 (aarch64 mode) and trying to boot a regular Linux distro (this example is OpenEmbedded based but generic issue, boot.scr just loads Image to $loadaddr and booti's):
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
389 bytes read in 2 ms (189.5 KiB/s)
## Executing script at 02000000
13298176 bytes read in 701 ms (18.1 MiB/s)
## Flattened Device Tree blob at 2effb500
    Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device TreeFDT creation failed! hanging...### ERROR ### Please RESET the board ###
Switching the board to using bootm_size rather than initrd_high/fdt_high=0xffffffff does resolve the issue and I can boot, but it's still a case we need to fix. Thanks!
Quoted 7 lines
Quoted 4 lines
Thanks for testing! Of course it's a case we need to fix! Would it be possible for you to do this run-time test again with the attached patch that enables DEBUG in lmb.c and dumps 'lmb' contents in the error case shown above?
Here. Note that I'm sure you can replicate this anywhere by setting initrd_high / fdt_high to 0xffffffff.

Quoted 42 lines

OK, thanks for this additional output. I cannot reproduce this in the tests though. I'll have to check this with my hardware probably, but I hope it's not related to 64 bit...

Regards,
Simon

Quoted 35 lines

U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
lmb_dump_all:

     memory.cnt                  = 0x1
     memory.size                 = 0x0
     memory.reg[0x0].base   = 0x0
                 .size   = 0x3b400000

     reserved.cnt        = 0x2
     reserved.size       = 0x0
     reserved.reg[0x0].base = 0x0
                   .size = 0x1000
     reserved.reg[0x1].base = 0x3af46e50
                   .size = 0x4b91b0
389 bytes read in 28 ms (12.7 KiB/s)
## Executing script at 02000000
lmb_dump_all:
     memory.cnt                  = 0x1
     memory.size                 = 0x0
     memory.reg[0x0].base   = 0x0
                 .size   = 0x3b400000

     reserved.cnt        = 0x2
     reserved.size       = 0x0
     reserved.reg[0x0].base = 0x0
                   .size = 0x1000
     reserved.reg[0x1].base = 0x3af46a20
                   .size = 0x4b95e0
13298176 bytes read in 746 ms (17 MiB/s)
## Flattened Device Tree blob at 2effb500
    Booting using the fdt blob at 0x2effb500

ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.

Tom, this looks strange. You said you're running this in 64-bit mode but here, fdt_high seems to be all ones in the lower 32 bit only. That's probably because you're running both 32bit and 64bit on the same target and that 'fdt_high' value got loaded with the environment?

Anyway, this was the reason I haven't found it. I did find it now and it's a bug that's been in lmb.c like forever. I'll send a v6 soon.

Regards,
Simon

Quoted 17 lines

Quoted 16 lines

Failed using fdt_high value for Device Treelmb_dump_all:

     memory.cnt                  = 0x1
     memory.size                 = 0x0
     memory.reg[0x0].base   = 0x0
                 .size   = 0x3b400000

     reserved.cnt        = 0x3
     reserved.size       = 0x0
     reserved.reg[0x0].base = 0x0
                   .size = 0x1000
     reserved.reg[0x1].base = 0x1080000
                   .size = 0xd83000
     reserved.reg[0x2].base = 0x3af46b10
                   .size = 0x4b94f0

FDT creation failed! hanging...### ERROR ### Please RESET the board ###

Tom Rini

2018-12-14 13:27 UTC #19 lore raw

v5 cover Fix CVE-2018-18440 and CVE-2018-18439

Quoted 113 lines · Simon Goldschmidt

Quoted 68 lines · Simon Goldschmidt
Am 11.12.2018 um 21:10 schrieb Tom Rini:
Quoted 18 lines · Simon Goldschmidt
Quoted 17 lines
Hi Tom,
[truncated the CC list a bit since I got "too many recipients" errors last time]
Am 11.12.2018 um 14:31 schrieb Tom Rini:
Quoted 11 lines · Simon Goldschmidt
Quoted 10 lines
This series fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by adding restrictions to the 'load' command and fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by adding restrictions to the tftp code. The functions from lmb.c are used to setup regions of allowed and reserved memory. Then, the file size to load is checked against these addresses and loading the file is aborted if it would overwrite reserved memory.
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
The memory reservation code is reused from bootm/image.
So, thanks for doing all of this. I'm sorry to dump a few problems on you now however. First, we have a lot of fail to builds:
https://travis-ci.org/trini/u-boot/builds/466333708
Ok, I'll check those. At first sight, the build errors seem to be identical in that 'fdt_get_resource' is missing. I'll check that config option.
The function 'fdt_get_resource' was the only match that I could find to decode register address + size from fdt. However, it resides in 'lib/fdtdec.c' which is only linked for OF_CONTROL. This seems strange as it is a read-access function to a dts and it can be used when booting, too. What would be the way to go here, move this to a different file or compile 'lib/fdtdec.c' depending on CONFIG_FIT or something?
2nd fail seems to be in 'test/py' tests. I'll dig into that, too.
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
Second, giving this a run-time test on Raspberry Pi 3 (aarch64 mode) and trying to boot a regular Linux distro (this example is OpenEmbedded based but generic issue, boot.scr just loads Image to $loadaddr and booti's):
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
389 bytes read in 2 ms (189.5 KiB/s)
## Executing script at 02000000
13298176 bytes read in 701 ms (18.1 MiB/s)
## Flattened Device Tree blob at 2effb500
    Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.
Failed using fdt_high value for Device TreeFDT creation failed! hanging...### ERROR ### Please RESET the board ###
Switching the board to using bootm_size rather than initrd_high/fdt_high=0xffffffff does resolve the issue and I can boot, but it's still a case we need to fix. Thanks!
Quoted 7 lines
Quoted 4 lines
Thanks for testing! Of course it's a case we need to fix! Would it be possible for you to do this run-time test again with the attached patch that enables DEBUG in lmb.c and dumps 'lmb' contents in the error case shown above?
Here. Note that I'm sure you can replicate this anywhere by setting initrd_high / fdt_high to 0xffffffff.
Quoted 42 lines
OK, thanks for this additional output. I cannot reproduce this in the tests though. I'll have to check this with my hardware probably, but I hope it's not related to 64 bit...
Regards,
Simon
Quoted 35 lines
U-Boot> run bootcmd
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
lmb_dump_all:
     memory.cnt                  = 0x1
     memory.size                 = 0x0
     memory.reg[0x0].base   = 0x0
                 .size   = 0x3b400000

     reserved.cnt        = 0x2
     reserved.size       = 0x0
     reserved.reg[0x0].base = 0x0
                   .size = 0x1000
     reserved.reg[0x1].base = 0x3af46e50
                   .size = 0x4b91b0
389 bytes read in 28 ms (12.7 KiB/s)
## Executing script at 02000000
lmb_dump_all:
     memory.cnt                  = 0x1
     memory.size                 = 0x0
     memory.reg[0x0].base   = 0x0
                 .size   = 0x3b400000

     reserved.cnt        = 0x2
     reserved.size       = 0x0
     reserved.reg[0x0].base = 0x0
                   .size = 0x1000
     reserved.reg[0x1].base = 0x3af46a20
                   .size = 0x4b95e0
13298176 bytes read in 746 ms (17 MiB/s)
## Flattened Device Tree blob at 2effb500
    Booting using the fdt blob at 0x2effb500
ERROR: Failed to allocate 0x7ab5 bytes below 0xffffffff.

Quoted 4 lines

Tom, this looks strange. You said you're running this in 64-bit mode but here, fdt_high seems to be all ones in the lower 32 bit only. That's probably because you're running both 32bit and 64bit on the same target and that 'fdt_high' value got loaded with the environment?

Yes, that's right. I run this Pi in both 32 and 64bit mode and the saved environment is from the 32bit side and fdt_high/initrd_high are, hah, oops, wrong.

Quoted 2 lines

Anyway, this was the reason I haven't found it. I did find it now and it's a bug that's been in lmb.c like forever. I'll send a v6 soon.

Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181214/9c297b22/attachment.sig>