[U-Boot] [PATCH v10 00/10] Fix CVE-2018-18440 and CVE-2018-18439

[U-Boot] [PATCH v10 00/10] Fix CVE-2018-18440 and CVE-2018-18439

[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.
Changes in v10:
- added acked-by and reviewed-by tags

Changes in v9:
- fixed compile error in patch 10/10 (in arch/arm/lib/bootm.c)

Changes in v8:
- fix address overflow in 'arch_lmb_reserve' for ARM

Changes in v7:
- add braces around if/else with macros accross more than one line
- fix compiling without CONFIG_FIT
- fix compiling without CONFIG_LMB

Changes in v6:
- fix size of allocated regions that need alignment padding
- fix compiling without OF_CONTROL
- fixed NULL pointer access in 'fdt_blob' passed to
  'boot_fdt_add_mem_rsv_regions'

Changes in v5:
- 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 ranges 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 (10):
  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
  arm: bootm: fix sp detection at end of address range

 arch/arm/lib/bootm.c |  10 +-
 common/bootm.c       |   8 +-
 common/image-fdt.c   |  53 +++-
 fs/fs.c              |  56 +++-
 include/lmb.h        |   7 +-
 lib/Makefile         |   1 +
 lib/lmb.c            | 106 ++++++--
 net/tftp.c           |  73 +++++-
 test/lib/Makefile    |   1 +
 test/lib/lmb.c       | 601 +++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 859 insertions(+), 57 deletions(-)
 create mode 100644 test/lib/lmb.c

-- 
2.17.1

[U-Boot] [PATCH v10 01/10] test: add test for lib/lmb.c

[Simon Goldschmidt]

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

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v10:
- add reviewed-by

Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5:
- this patch is new in v5

Changes in v4: None
Changes in v2: None

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

diff --git a/test/lib/Makefile b/test/lib/Makefile
index ea68fae566..5a636aac74 100644
--- a/test/lib/Makefile
+++ b/test/lib/Makefile
@@ -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
diff --git a/test/lib/lmb.c b/test/lib/lmb.c
new file mode 100644
index 0000000000..dd7ba14b34
--- /dev/null
+++ b/test/lib/lmb.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2018 Simon Goldschmidt
+ */
+
+#include <common.h>
+#include <lmb.h>
+#include <dm/test.h>
+#include <test/ut.h>
+
+static int check_lmb(struct unit_test_state *uts, struct lmb *lmb,
+		     phys_addr_t ram_base, phys_size_t ram_size,
+		     unsigned long num_reserved,
+		     phys_addr_t base1, phys_size_t size1,
+		     phys_addr_t base2, phys_size_t size2,
+		     phys_addr_t base3, phys_size_t size3)
+{
+	ut_asserteq(lmb->memory.cnt, 1);
+	ut_asserteq(lmb->memory.region[0].base, ram_base);
+	ut_asserteq(lmb->memory.region[0].size, ram_size);
+
+	ut_asserteq(lmb->reserved.cnt, num_reserved);
+	if (num_reserved > 0) {
+		ut_asserteq(lmb->reserved.region[0].base, base1);
+		ut_asserteq(lmb->reserved.region[0].size, size1);
+	}
+	if (num_reserved > 1) {
+		ut_asserteq(lmb->reserved.region[1].base, base2);
+		ut_asserteq(lmb->reserved.region[1].size, size2);
+	}
+	if (num_reserved > 2) {
+		ut_asserteq(lmb->reserved.region[2].base, base3);
+		ut_asserteq(lmb->reserved.region[2].size, size3);
+	}
+	return 0;
+}
+
+#define ASSERT_LMB(lmb, ram_base, ram_size, num_reserved, base1, size1, \
+		   base2, size2, base3, size3) \
+		   ut_assert(!check_lmb(uts, lmb, ram_base, ram_size, \
+			     num_reserved, base1, size1, base2, size2, base3, \
+			     size3))
+
+/*
+ * Test helper function that reserves 64 KiB somewhere in the simulated RAM and
+ * then does some alloc + free tests.
+ */
+static int test_multi_alloc(struct unit_test_state *uts,
+			    const phys_addr_t ram, const phys_size_t ram_size,
+			    const phys_addr_t alloc_64k_addr)
+{
+	const phys_addr_t ram_end = ram + ram_size;
+	const phys_addr_t alloc_64k_end = alloc_64k_addr + 0x10000;
+
+	struct lmb lmb;
+	long ret;
+	phys_addr_t a, a2, b, b2, c, d;
+
+	/* check for overflow */
+	ut_assert(ram_end == 0 || ram_end > ram);
+	ut_assert(alloc_64k_end > alloc_64k_addr);
+	/* check input addresses + size */
+	ut_assert(alloc_64k_addr >= ram + 8);
+	ut_assert(alloc_64k_end <= ram_end - 8);
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/* reserve 64KiB somewhere */
+	ret = lmb_reserve(&lmb, alloc_64k_addr, 0x10000);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, alloc_64k_addr, 0x10000,
+		   0, 0, 0, 0);
+
+	/* allocate somewhere, should be@the end of RAM */
+	a = lmb_alloc(&lmb, 4, 1);
+	ut_asserteq(a, ram_end - 4);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, alloc_64k_addr, 0x10000,
+		   ram_end - 4, 4, 0, 0);
+	/* alloc below end of reserved region -> below reserved region */
+	b = lmb_alloc_base(&lmb, 4, 1, alloc_64k_end);
+	ut_asserteq(b, alloc_64k_addr - 4);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 4, 0x10000 + 4, ram_end - 4, 4, 0, 0);
+
+	/* 2nd time */
+	c = lmb_alloc(&lmb, 4, 1);
+	ut_asserteq(c, ram_end - 8);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 4, 0x10000 + 4, ram_end - 8, 8, 0, 0);
+	d = lmb_alloc_base(&lmb, 4, 1, alloc_64k_end);
+	ut_asserteq(d, alloc_64k_addr - 8);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 8, 0, 0);
+
+	ret = lmb_free(&lmb, a, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 4, 0, 0);
+	/* allocate again to ensure we get the same address */
+	a2 = lmb_alloc(&lmb, 4, 1);
+	ut_asserteq(a, a2);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 8, 0, 0);
+	ret = lmb_free(&lmb, a2, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 4, 0, 0);
+
+	ret = lmb_free(&lmb, b, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 3,
+		   alloc_64k_addr - 8, 4, alloc_64k_addr, 0x10000,
+		   ram_end - 8, 4);
+	/* allocate again to ensure we get the same address */
+	b2 = lmb_alloc_base(&lmb, 4, 1, alloc_64k_end);
+	ut_asserteq(b, b2);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 4, 0, 0);
+	ret = lmb_free(&lmb, b2, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 3,
+		   alloc_64k_addr - 8, 4, alloc_64k_addr, 0x10000,
+		   ram_end - 8, 4);
+
+	ret = lmb_free(&lmb, c, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 2,
+		   alloc_64k_addr - 8, 4, alloc_64k_addr, 0x10000, 0, 0);
+	ret = lmb_free(&lmb, d, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, alloc_64k_addr, 0x10000,
+		   0, 0, 0, 0);
+
+	return 0;
+}
+
+static int test_multi_alloc_512mb(struct unit_test_state *uts,
+				  const phys_addr_t ram)
+{
+	return test_multi_alloc(uts, ram, 0x20000000, ram + 0x10000000);
+}
+
+/* Create a memory region with one reserved region and allocate */
+static int lib_test_lmb_simple(struct unit_test_state *uts)
+{
+	/* simulate 512 MiB RAM beginning at 1GiB */
+	return test_multi_alloc_512mb(uts, 0x40000000);
+}
+
+DM_TEST(lib_test_lmb_simple, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Simulate 512 MiB RAM, allocate some blocks that fit/don't fit */
+static int test_bigblock(struct unit_test_state *uts, const phys_addr_t ram)
+{
+	const phys_size_t ram_size = 0x20000000;
+	const phys_size_t big_block_size = 0x10000000;
+	const phys_addr_t ram_end = ram + ram_size;
+	const phys_addr_t alloc_64k_addr = ram + 0x10000000;
+	struct lmb lmb;
+	long ret;
+	phys_addr_t a, b;
+
+	/* check for overflow */
+	ut_assert(ram_end == 0 || ram_end > ram);
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/* reserve 64KiB in the middle of RAM */
+	ret = lmb_reserve(&lmb, alloc_64k_addr, 0x10000);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, alloc_64k_addr, 0x10000,
+		   0, 0, 0, 0);
+
+	/* allocate a big block, should be below reserved */
+	a = lmb_alloc(&lmb, big_block_size, 1);
+	ut_asserteq(a, ram);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, a,
+		   big_block_size + 0x10000, 0, 0, 0, 0);
+	/* allocate 2nd big block */
+	/* This should fail, printing an error */
+	b = lmb_alloc(&lmb, big_block_size, 1);
+	ut_asserteq(b, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, a,
+		   big_block_size + 0x10000, 0, 0, 0, 0);
+
+	ret = lmb_free(&lmb, a, big_block_size);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, alloc_64k_addr, 0x10000,
+		   0, 0, 0, 0);
+
+	/* allocate too big block */
+	/* This should fail, printing an error */
+	a = lmb_alloc(&lmb, ram_size, 1);
+	ut_asserteq(a, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, alloc_64k_addr, 0x10000,
+		   0, 0, 0, 0);
+
+	return 0;
+}
+
+static int lib_test_lmb_big(struct unit_test_state *uts)
+{
+	return test_bigblock(uts, 0x40000000);
+}
+
+DM_TEST(lib_test_lmb_big, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Simulate 512 MiB RAM, allocate a block without previous reservation */
+static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram)
+{
+	const phys_size_t ram_size = 0x20000000;
+	const phys_addr_t ram_end = ram + ram_size;
+	struct lmb lmb;
+	long ret;
+	phys_addr_t a, b;
+
+	/* check for overflow */
+	ut_assert(ram_end == 0 || ram_end > ram);
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/* allocate a block */
+	a = lmb_alloc(&lmb, 4, 1);
+	ut_assert(a != 0);
+	/* and free it */
+	ret = lmb_free(&lmb, a, 4);
+	ut_asserteq(ret, 0);
+
+	/* allocate a block with base*/
+	b = lmb_alloc_base(&lmb, 4, 1, ram_end);
+	ut_assert(a == b);
+	/* and free it */
+	ret = lmb_free(&lmb, b, 4);
+	ut_asserteq(ret, 0);
+
+	return 0;
+}
+
+static int lib_test_lmb_noreserved(struct unit_test_state *uts)
+{
+	return test_noreserved(uts, 0x40000000);
+}
+
+DM_TEST(lib_test_lmb_noreserved, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/*
+ * Simulate a RAM that starts at 0 and allocate down to address 0, which must
+ * fail as '0' means failure for the lmb_alloc functions.
+ */
+static int lib_test_lmb_at_0(struct unit_test_state *uts)
+{
+	const phys_addr_t ram = 0;
+	const phys_size_t ram_size = 0x20000000;
+	struct lmb lmb;
+	long ret;
+	phys_addr_t a, b;
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/* allocate nearly everything */
+	a = lmb_alloc(&lmb, ram_size - 4, 1);
+	ut_asserteq(a, ram + 4);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, a, ram_size - 4,
+		   0, 0, 0, 0);
+	/* allocate the rest */
+	/* This should fail as the allocated address would be 0 */
+	b = lmb_alloc(&lmb, 4, 1);
+	ut_asserteq(b, 0);
+	/* check that this was an error by checking lmb */
+	ASSERT_LMB(&lmb, ram, ram_size, 1, a, ram_size - 4,
+		   0, 0, 0, 0);
+	/* check that this was an error by freeing b */
+	ret = lmb_free(&lmb, b, 4);
+	ut_asserteq(ret, -1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, a, ram_size - 4,
+		   0, 0, 0, 0);
+
+	ret = lmb_free(&lmb, a, ram_size - 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
+
+	return 0;
+}
+
+DM_TEST(lib_test_lmb_at_0, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.17.1

[U-Boot] [PATCH v10 02/10] lmb: fix allocation at end of address range

[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 <simon.k.r.goldschmidt@gmail.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5:
- this patch is new in v5

Changes in v4: None
Changes in v2: None

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

diff --git a/lib/lmb.c b/lib/lmb.c
index 1705417348..6d3dcf4e09 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -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)
diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index dd7ba14b34..fb7ca45ef1 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -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);
-- 
2.17.1

[U-Boot] [PATCH v10 03/10] lib: lmb: reserving overlapping regions should fail

[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.

Also, to keep reserved memory correct after 'free', reserved entries
created by allocating memory must not set their size to a multiple
of alignment but to the original size. This ensures the reserved
region is completely removed when the caller calls 'lmb_free', as
this one takes the same size as passed to 'lmb_alloc' etc.

Add test to assert this.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7:
- add braces around if/else with macros accross more than one line

Changes in v6:
- fix size of allocated regions that need alignment padding

Changes in v5:
- added a test for this bug

Changes in v4: None
Changes in v2: None

 lib/lmb.c      | 11 +++---
 test/lib/lmb.c | 95 +++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index 6d3dcf4e09..cd297f8202 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -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;
 		}
 	}
 
@@ -269,11 +272,6 @@ static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size)
 	return addr & ~(size - 1);
 }
 
-static phys_addr_t lmb_align_up(phys_addr_t addr, ulong size)
-{
-	return (addr + (size - 1)) & ~(size - 1);
-}
-
 phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phys_addr_t max_addr)
 {
 	long i, j;
@@ -302,8 +300,7 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
 			if (j < 0) {
 				/* This area isn't reserved, take it */
 				if (lmb_add_region(&lmb->reserved, base,
-							lmb_align_up(size,
-								align)) < 0)
+						   size) < 0)
 					return 0;
 				return base;
 			}
diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index fb7ca45ef1..e6acb70e76 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -227,13 +227,16 @@ static int lib_test_lmb_big(struct unit_test_state *uts)
 DM_TEST(lib_test_lmb_big, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
 
 /* Simulate 512 MiB RAM, allocate a block without previous reservation */
-static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram)
+static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram,
+			   const phys_addr_t alloc_size, const ulong align)
 {
 	const phys_size_t ram_size = 0x20000000;
 	const phys_addr_t ram_end = ram + ram_size;
 	struct lmb lmb;
 	long ret;
 	phys_addr_t a, b;
+	const phys_addr_t alloc_size_aligned = (alloc_size + align - 1) &
+		~(align - 1);
 
 	/* check for overflow */
 	ut_assert(ram_end == 0 || ram_end > ram);
@@ -242,20 +245,43 @@ static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram)
 
 	ret = lmb_add(&lmb, ram, ram_size);
 	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
 
 	/* allocate a block */
-	a = lmb_alloc(&lmb, 4, 1);
+	a = lmb_alloc(&lmb, alloc_size, align);
 	ut_assert(a != 0);
-	/* and free it */
-	ret = lmb_free(&lmb, a, 4);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram + ram_size - alloc_size_aligned,
+		   alloc_size, 0, 0, 0, 0);
+	/* allocate another block */
+	b = lmb_alloc(&lmb, alloc_size, align);
+	ut_assert(b != 0);
+	if (alloc_size == alloc_size_aligned) {
+		ASSERT_LMB(&lmb, ram, ram_size, 1, ram + ram_size -
+			   (alloc_size_aligned * 2), alloc_size * 2, 0, 0, 0,
+			   0);
+	} else {
+		ASSERT_LMB(&lmb, ram, ram_size, 2, ram + ram_size -
+			   (alloc_size_aligned * 2), alloc_size, ram + ram_size
+			   - alloc_size_aligned, alloc_size, 0, 0);
+	}
+	/* and free them */
+	ret = lmb_free(&lmb, b, alloc_size);
 	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram + ram_size - alloc_size_aligned,
+		   alloc_size, 0, 0, 0, 0);
+	ret = lmb_free(&lmb, a, alloc_size);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
 
 	/* allocate a block with base*/
-	b = lmb_alloc_base(&lmb, 4, 1, ram_end);
+	b = lmb_alloc_base(&lmb, alloc_size, align, ram_end);
 	ut_assert(a == b);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram + ram_size - alloc_size_aligned,
+		   alloc_size, 0, 0, 0, 0);
 	/* and free it */
-	ret = lmb_free(&lmb, b, 4);
+	ret = lmb_free(&lmb, b, alloc_size);
 	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
 
 	return 0;
 }
@@ -265,16 +291,30 @@ static int lib_test_lmb_noreserved(struct unit_test_state *uts)
 	int ret;
 
 	/* simulate 512 MiB RAM beginning at 1GiB */
-	ret = test_noreserved(uts, 0x40000000);
+	ret = test_noreserved(uts, 0x40000000, 4, 1);
 	if (ret)
 		return ret;
 
 	/* simulate 512 MiB RAM beginning at 1.5GiB */
-	return test_noreserved(uts, 0xE0000000);
+	return test_noreserved(uts, 0xE0000000, 4, 1);
 }
 
 DM_TEST(lib_test_lmb_noreserved, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
 
+static int lib_test_lmb_unaligned_size(struct unit_test_state *uts)
+{
+	int ret;
+
+	/* simulate 512 MiB RAM beginning at 1GiB */
+	ret = test_noreserved(uts, 0x40000000, 5, 8);
+	if (ret)
+		return ret;
+
+	/* simulate 512 MiB RAM beginning at 1.5GiB */
+	return test_noreserved(uts, 0xE0000000, 5, 8);
+}
+
+DM_TEST(lib_test_lmb_unaligned_size, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
 /*
  * Simulate a RAM that starts at 0 and allocate down to address 0, which must
  * fail as '0' means failure for the lmb_alloc functions.
@@ -318,3 +358,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);
-- 
2.17.1

[U-Boot] [PATCH v10 04/10] fdt: parse "reserved-memory" for memory reservation

[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 <simon.k.r.goldschmidt@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v10:
- add reviewed-by

Changes in v9: None
Changes in v8: None
Changes in v7:
- fix compiling without CONFIG_FIT

Changes in v6:
- fix compiling without OF_CONTROL

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 +++++++++++++++++++++++++++++++++++++++-------
 lib/Makefile       |  1 +
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/common/image-fdt.c b/common/image-fdt.c
index 95748f0ae1..5c0d6db3fe 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -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);
+		}
 	}
 }
 
diff --git a/lib/Makefile b/lib/Makefile
index a6dd928a92..358789ff12 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -30,6 +30,7 @@ obj-y += crc7.o
 obj-y += crc8.o
 obj-y += crc16.o
 obj-$(CONFIG_ERRNO_STR) += errno_str.o
+obj-$(CONFIG_OF_LIBFDT) += fdtdec.o
 obj-$(CONFIG_FIT) += fdtdec_common.o
 obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
 obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o
-- 
2.17.1

[U-Boot] [PATCH v10 05/10] lib: lmb: extend lmb for checks at load time

[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 <simon.k.r.goldschmidt@gmail.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
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(+)

diff --git a/include/lmb.h b/include/lmb.h
index f04d058093..7d7e2a78dc 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -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);
 
diff --git a/lib/lmb.c b/lib/lmb.c
index cd297f8202..e380a0a722 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
 	return 0;
 }
 
+/*
+ * Try to allocate a specific address range: must be in defined memory but not
+ * reserved
+ */
+phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size)
+{
+	long j;
+
+	/* Check if the requested address is in one of the memory regions */
+	j = lmb_overlaps_region(&lmb->memory, base, size);
+	if (j >= 0) {
+		/*
+		 * Check if the requested end address is in the same memory
+		 * region we found.
+		 */
+		if (lmb_addrs_overlap(lmb->memory.region[j].base,
+				      lmb->memory.region[j].size, base + size -
+				      1, 1)) {
+			/* ok, reserve the memory */
+			if (lmb_reserve(lmb, base, size) >= 0)
+				return base;
+		}
+	}
+	return 0;
+}
+
+/* Return number of bytes from a given address that are free */
+phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)
+{
+	int i;
+	long j;
+
+	/* check if the requested address is in the memory regions */
+	j = lmb_overlaps_region(&lmb->memory, addr, 1);
+	if (j >= 0) {
+		for (i = 0; i < lmb->reserved.cnt; i++) {
+			if (addr < lmb->reserved.region[i].base) {
+				/* first reserved range > requested address */
+				return lmb->reserved.region[i].base - addr;
+			}
+			if (lmb->reserved.region[i].base +
+			    lmb->reserved.region[i].size > addr) {
+				/* requested addr is in this reserved range */
+				return 0;
+			}
+		}
+		/* if we come here: no reserved ranges above requested addr */
+		return lmb->memory.region[lmb->memory.cnt - 1].base +
+		       lmb->memory.region[lmb->memory.cnt - 1].size - addr;
+	}
+	return 0;
+}
+
 int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
 {
 	int i;
diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index e6acb70e76..058d3c332b 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -397,3 +397,205 @@ static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts)
 
 DM_TEST(lib_test_lmb_overlapping_reserve,
 	DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/*
+ * Simulate 512 MiB RAM, reserve 3 blocks, allocate addresses in between.
+ * Expect addresses outside the memory range to fail.
+ */
+static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
+{
+	const phys_size_t ram_size = 0x20000000;
+	const phys_addr_t ram_end = ram + ram_size;
+	const phys_size_t alloc_addr_a = ram + 0x8000000;
+	const phys_size_t alloc_addr_b = ram + 0x8000000 * 2;
+	const phys_size_t alloc_addr_c = ram + 0x8000000 * 3;
+	struct lmb lmb;
+	long ret;
+	phys_addr_t a, b, c, d, e;
+
+	/* check for overflow */
+	ut_assert(ram_end == 0 || ram_end > ram);
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/*  reserve 3 blocks */
+	ret = lmb_reserve(&lmb, alloc_addr_a, 0x10000);
+	ut_asserteq(ret, 0);
+	ret = lmb_reserve(&lmb, alloc_addr_b, 0x10000);
+	ut_asserteq(ret, 0);
+	ret = lmb_reserve(&lmb, alloc_addr_c, 0x10000);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, alloc_addr_a, 0x10000,
+		   alloc_addr_b, 0x10000, alloc_addr_c, 0x10000);
+
+	/* allocate blocks */
+	a = lmb_alloc_addr(&lmb, ram, alloc_addr_a - ram);
+	ut_asserteq(a, ram);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, ram, 0x8010000,
+		   alloc_addr_b, 0x10000, alloc_addr_c, 0x10000);
+	b = lmb_alloc_addr(&lmb, alloc_addr_a + 0x10000,
+			   alloc_addr_b - alloc_addr_a - 0x10000);
+	ut_asserteq(b, alloc_addr_a + 0x10000);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, ram, 0x10010000,
+		   alloc_addr_c, 0x10000, 0, 0);
+	c = lmb_alloc_addr(&lmb, alloc_addr_b + 0x10000,
+			   alloc_addr_c - alloc_addr_b - 0x10000);
+	ut_asserteq(c, alloc_addr_b + 0x10000);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
+		   0, 0, 0, 0);
+	d = lmb_alloc_addr(&lmb, alloc_addr_c + 0x10000,
+			   ram_end - alloc_addr_c - 0x10000);
+	ut_asserteq(d, alloc_addr_c + 0x10000);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, ram_size,
+		   0, 0, 0, 0);
+
+	/* allocating anything else should fail */
+	e = lmb_alloc(&lmb, 1, 1);
+	ut_asserteq(e, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, ram_size,
+		   0, 0, 0, 0);
+
+	ret = lmb_free(&lmb, d, ram_end - alloc_addr_c - 0x10000);
+	ut_asserteq(ret, 0);
+
+	/* allocate at 3 points in free range */
+
+	d = lmb_alloc_addr(&lmb, ram_end - 4, 4);
+	ut_asserteq(d, ram_end - 4);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, ram, 0x18010000,
+		   d, 4, 0, 0);
+	ret = lmb_free(&lmb, d, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
+		   0, 0, 0, 0);
+
+	d = lmb_alloc_addr(&lmb, ram_end - 128, 4);
+	ut_asserteq(d, ram_end - 128);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, ram, 0x18010000,
+		   d, 4, 0, 0);
+	ret = lmb_free(&lmb, d, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
+		   0, 0, 0, 0);
+
+	d = lmb_alloc_addr(&lmb, alloc_addr_c + 0x10000, 4);
+	ut_asserteq(d, alloc_addr_c + 0x10000);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010004,
+		   0, 0, 0, 0);
+	ret = lmb_free(&lmb, d, 4);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram, 0x18010000,
+		   0, 0, 0, 0);
+
+	/* allocate at the bottom */
+	ret = lmb_free(&lmb, a, alloc_addr_a - ram);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, ram + 0x8000000, 0x10010000,
+		   0, 0, 0, 0);
+	d = lmb_alloc_addr(&lmb, ram, 4);
+	ut_asserteq(d, ram);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, d, 4,
+		   ram + 0x8000000, 0x10010000, 0, 0);
+
+	/* check that allocating outside memory fails */
+	if (ram_end != 0) {
+		ret = lmb_alloc_addr(&lmb, ram_end, 1);
+		ut_asserteq(ret, 0);
+	}
+	if (ram != 0) {
+		ret = lmb_alloc_addr(&lmb, ram - 1, 1);
+		ut_asserteq(ret, 0);
+	}
+
+	return 0;
+}
+
+static int lib_test_lmb_alloc_addr(struct unit_test_state *uts)
+{
+	int ret;
+
+	/* simulate 512 MiB RAM beginning at 1GiB */
+	ret = test_alloc_addr(uts, 0x40000000);
+	if (ret)
+		return ret;
+
+	/* simulate 512 MiB RAM beginning at 1.5GiB */
+	return test_alloc_addr(uts, 0xE0000000);
+}
+
+DM_TEST(lib_test_lmb_alloc_addr, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Simulate 512 MiB RAM, reserve 3 blocks, check addresses in between */
+static int test_get_unreserved_size(struct unit_test_state *uts,
+				    const phys_addr_t ram)
+{
+	const phys_size_t ram_size = 0x20000000;
+	const phys_addr_t ram_end = ram + ram_size;
+	const phys_size_t alloc_addr_a = ram + 0x8000000;
+	const phys_size_t alloc_addr_b = ram + 0x8000000 * 2;
+	const phys_size_t alloc_addr_c = ram + 0x8000000 * 3;
+	struct lmb lmb;
+	long ret;
+	phys_size_t s;
+
+	/* check for overflow */
+	ut_assert(ram_end == 0 || ram_end > ram);
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/*  reserve 3 blocks */
+	ret = lmb_reserve(&lmb, alloc_addr_a, 0x10000);
+	ut_asserteq(ret, 0);
+	ret = lmb_reserve(&lmb, alloc_addr_b, 0x10000);
+	ut_asserteq(ret, 0);
+	ret = lmb_reserve(&lmb, alloc_addr_c, 0x10000);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, alloc_addr_a, 0x10000,
+		   alloc_addr_b, 0x10000, alloc_addr_c, 0x10000);
+
+	/* check addresses in between blocks */
+	s = lmb_get_unreserved_size(&lmb, ram);
+	ut_asserteq(s, alloc_addr_a - ram);
+	s = lmb_get_unreserved_size(&lmb, ram + 0x10000);
+	ut_asserteq(s, alloc_addr_a - ram - 0x10000);
+	s = lmb_get_unreserved_size(&lmb, alloc_addr_a - 4);
+	ut_asserteq(s, 4);
+
+	s = lmb_get_unreserved_size(&lmb, alloc_addr_a + 0x10000);
+	ut_asserteq(s, alloc_addr_b - alloc_addr_a - 0x10000);
+	s = lmb_get_unreserved_size(&lmb, alloc_addr_a + 0x20000);
+	ut_asserteq(s, alloc_addr_b - alloc_addr_a - 0x20000);
+	s = lmb_get_unreserved_size(&lmb, alloc_addr_b - 4);
+	ut_asserteq(s, 4);
+
+	s = lmb_get_unreserved_size(&lmb, alloc_addr_c + 0x10000);
+	ut_asserteq(s, ram_end - alloc_addr_c - 0x10000);
+	s = lmb_get_unreserved_size(&lmb, alloc_addr_c + 0x20000);
+	ut_asserteq(s, ram_end - alloc_addr_c - 0x20000);
+	s = lmb_get_unreserved_size(&lmb, ram_end - 4);
+	ut_asserteq(s, 4);
+
+	return 0;
+}
+
+static int lib_test_lmb_get_unreserved_size(struct unit_test_state *uts)
+{
+	int ret;
+
+	/* simulate 512 MiB RAM beginning at 1GiB */
+	ret = test_get_unreserved_size(uts, 0x40000000);
+	if (ret)
+		return ret;
+
+	/* simulate 512 MiB RAM beginning at 1.5GiB */
+	return test_get_unreserved_size(uts, 0xE0000000);
+}
+
+DM_TEST(lib_test_lmb_get_unreserved_size,
+	DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.17.1

[U-Boot] [PATCH v10 06/10] fs: prevent overwriting reserved memory

[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 <simon.k.r.goldschmidt@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v10:
- return -ENOSPC from fs_read when read to reserved memory is rejected
- add reviewed-by

Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6:
- fixed NULL pointer access in 'fdt_blob' passed to
  'boot_fdt_add_mem_rsv_regions'

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(-)

diff --git a/fs/fs.c b/fs/fs.c
index cb265174e2..7fd22101ef 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -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 -ENOSPC;
+}
+#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;
diff --git a/include/lmb.h b/include/lmb.h
index 7d7e2a78dc..62da85e716 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -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);
diff --git a/lib/lmb.c b/lib/lmb.c
index e380a0a722..3407705fa7 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -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 && fdt_blob)
+		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)
 {
-- 
2.17.1

[U-Boot] [PATCH v10 07/10] bootm: use new common function lmb_init_and_reserve

[Simon Goldschmidt]

This reduces duplicate code only.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v10:
- add reviewed-by

Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v2: None

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

diff --git a/common/bootm.c b/common/bootm.c
index 8bf84ebcb7..31e4f0f794 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -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)
-- 
2.17.1

[U-Boot] [PATCH v10 08/10] lmb: remove unused extern declaration

[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 <simon.k.r.goldschmidt@gmail.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
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(-)

diff --git a/include/lmb.h b/include/lmb.h
index 62da85e716..1bb003e35e 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -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);
-- 
2.17.1

[U-Boot] [PATCH v10 09/10] tftp: prevent overwriting reserved memory

[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 <simon.k.r.goldschmidt@gmail.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
---

Changes in v10:
- add acked-by

Changes in v9: None
Changes in v8: None
Changes in v7:
- fix compiling without CONFIG_LMB

Changes in v6: None
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 | 73 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 10 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index 68ffd81414..a9335b1b7e 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -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,10 @@ 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;
+#ifdef CONFIG_LMB
+static ulong	tftp_load_size;
+#endif
 #ifdef CONFIG_TFTP_TSIZE
 /* The file size reported by the server */
 static int	tftp_tsize;
@@ -164,10 +170,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 +182,32 @@ 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;
+
+#ifdef CONFIG_LMB
+		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;
+		}
+#endif
+		ptr = map_sysmem(store_addr, len);
 		memcpy(ptr, src, len);
 		unmap_sysmem(ptr);
 	}
@@ -203,6 +218,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 +623,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 +716,25 @@ 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)
+{
+#ifdef CONFIG_LMB
+	struct lmb lmb;
+	phys_size_t max_size;
+
+	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, load_addr);
+	if (!max_size)
+		return -1;
+
+	tftp_load_size = max_size;
+#endif
+	tftp_load_addr = load_addr;
+	return 0;
+}
 
 void tftp_start(enum proto_t protocol)
 {
@@ -791,7 +831,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 +889,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");
 
-- 
2.17.1

[U-Boot] [PATCH v10 10/10] arm: bootm: fix sp detection at end of address range

[Simon Goldschmidt]

This fixes  'arch_lmb_reserve()' for ARM that tries to detect in which
DRAM bank 'sp' is in.

This code failed if a bank was at the end of physical address range
(i.e. size + length overflowed to 0).

To fix this, calculate 'bank_end' as 'size + length - 1' so that such
banks end at 0xffffffff, not 0.

Fixes: 15751403b6 ("ARM: bootm: don't assume sp is in DRAM bank 0")
Reported-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Reviewed-by: Stephen Warren <swarren@nvidia.com>
---

Changes in v10: None
Changes in v9:
- fix compile error in arch/arm/lib/bootm.c

Changes in v8:
- this patch is new in v8

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v2: None

 arch/arm/lib/bootm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index c3c1d2fdfa..329f20c2bf 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -64,13 +64,15 @@ void arch_lmb_reserve(struct lmb *lmb)
 	/* adjust sp by 4K to be safe */
 	sp -= 4096;
 	for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
-		if (sp < gd->bd->bi_dram[bank].start)
+		if (!gd->bd->bi_dram[bank].size ||
+		    sp < gd->bd->bi_dram[bank].start)
 			continue;
+		/* Watch out for RAM at end of address space! */
 		bank_end = gd->bd->bi_dram[bank].start +
-			gd->bd->bi_dram[bank].size;
-		if (sp >= bank_end)
+			gd->bd->bi_dram[bank].size - 1;
+		if (sp > bank_end)
 			continue;
-		lmb_reserve(lmb, sp, bank_end - sp);
+		lmb_reserve(lmb, sp, bank_end - sp + 1);
 		break;
 	}
 }
-- 
2.17.1

[U-Boot] [PATCH v10 00/10] Fix CVE-2018-18440 and CVE-2018-18439

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:13PM +0100, Simon Goldschmidt wrote:

> 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 v10:
> - added acked-by and reviewed-by tags

Note that patchwork collects these automatically and we don't need to
re-post things just for tags.  Was anything else changed?  Thanks!

-- 
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/20190114/2f1d24e0/attachment.sig>

[U-Boot] [PATCH v10 00/10] Fix CVE-2018-18440 and CVE-2018-18439

[Simon Goldschmidt]

Am Mo., 14. Jan. 2019, 23:55 hat Tom Rini <trini@konsulko.com> geschrieben:

> On Mon, Jan 14, 2019 at 10:38:13PM +0100, Simon Goldschmidt wrote:
>
> > 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 v10:
> > - added acked-by and reviewed-by tags
>
> Note that patchwork collects these automatically and we don't need to
> re-post things just for tags.  Was anything else changed?  Thanks!
>

Yes, I changed a return value in patch 6/10. I wouldn't have resend it
otherwise. But anyway, patchwork did not seem to catch Simon's
reviewed-by...

Regards,
Simon

>

[U-Boot] [PATCH v10 02/10] lmb: fix allocation at end of address range

[Simon Glass]

On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> 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 <simon.k.r.goldschmidt@gmail.com>
> ---
>
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5:
> - this patch is new in v5
>
> Changes in v4: None
> Changes in v2: None
>
>  lib/lmb.c      | 29 ++++++++++++-----------------
>  test/lib/lmb.c | 29 ++++++++++++++++++++++++++---
>  2 files changed, 38 insertions(+), 20 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v10 03/10] lib: lmb: reserving overlapping regions should fail

[Simon Glass]

On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> 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.
>
> Also, to keep reserved memory correct after 'free', reserved entries
> created by allocating memory must not set their size to a multiple
> of alignment but to the original size. This ensures the reserved
> region is completely removed when the caller calls 'lmb_free', as
> this one takes the same size as passed to 'lmb_alloc' etc.
>
> Add test to assert this.
>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
>
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7:
> - add braces around if/else with macros accross more than one line
>
> Changes in v6:
> - fix size of allocated regions that need alignment padding
>
> Changes in v5:
> - added a test for this bug
>
> Changes in v4: None
> Changes in v2: None
>
>  lib/lmb.c      | 11 +++---
>  test/lib/lmb.c | 95 +++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 91 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v10 05/10] lib: lmb: extend lmb for checks at load time

[Simon Glass]

Hi Simon,

On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> 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 <simon.k.r.goldschmidt@gmail.com>
> ---
>
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> 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(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

But please see suggestions/nits below.

>
> diff --git a/include/lmb.h b/include/lmb.h
> index f04d058093..7d7e2a78dc 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -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);

Can you please add full comments in the header for new functions.

> +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);
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index cd297f8202..e380a0a722 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
>         return 0;
>  }
>
> +/*
> + * Try to allocate a specific address range: must be in defined memory but not
> + * reserved
> + */
> +phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size)
> +{
> +       long j;

How about addr instead of j? I think single-char vars are OK for loop
counters, etc. but this is not that.

> +
> +       /* Check if the requested address is in one of the memory regions */
> +       j = lmb_overlaps_region(&lmb->memory, base, size);
> +       if (j >= 0) {
> +               /*
> +                * Check if the requested end address is in the same memory
> +                * region we found.
> +                */
> +               if (lmb_addrs_overlap(lmb->memory.region[j].base,
> +                                     lmb->memory.region[j].size, base + size -
> +                                     1, 1)) {
> +                       /* ok, reserve the memory */
> +                       if (lmb_reserve(lmb, base, size) >= 0)
> +                               return base;
> +               }
> +       }
> +       return 0;
> +}
> +
> +/* Return number of bytes from a given address that are free */
> +phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)

I support you use 'unreserved' instead of 'free' due to some subtle
difference in meaning? Can you add a comment somewhere about this?

> +{
> +       int i;
> +       long j;

Here too - addr?

> +
> +       /* check if the requested address is in the memory regions */
> +       j = lmb_overlaps_region(&lmb->memory, addr, 1);
> +       if (j >= 0) {
> +               for (i = 0; i < lmb->reserved.cnt; i++) {
> +                       if (addr < lmb->reserved.region[i].base) {
> +                               /* first reserved range > requested address */
> +                               return lmb->reserved.region[i].base - addr;
> +                       }
> +                       if (lmb->reserved.region[i].base +
> +                           lmb->reserved.region[i].size > addr) {
> +                               /* requested addr is in this reserved range */
> +                               return 0;
> +                       }
> +               }
> +               /* if we come here: no reserved ranges above requested addr */
> +               return lmb->memory.region[lmb->memory.cnt - 1].base +
> +                      lmb->memory.region[lmb->memory.cnt - 1].size - addr;
> +       }
> +       return 0;
> +}
> +

Regards,
Simon

[U-Boot] [PATCH v10 08/10] lmb: remove unused extern declaration

[Simon Glass]

On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> lmb.h includes an extern declaration of "struct lmb lmb;" which
> is not used anywhere, so remove it.
>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
>
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Great!
\

[U-Boot] [PATCH v10 05/10] lib: lmb: extend lmb for checks at load time

[Simon Goldschmidt]

Am Mi., 16. Jan. 2019, 22:34 hat Simon Glass <sjg@chromium.org> geschrieben:

> Hi Simon,
>
> On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
> >
> > 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 <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> > Changes in v10: None
> > Changes in v9: None
> > Changes in v8: None
> > Changes in v7: None
> > Changes in v6: None
> > 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(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But please see suggestions/nits below.
>
> >
> > diff --git a/include/lmb.h b/include/lmb.h
> > index f04d058093..7d7e2a78dc 100644
> > --- a/include/lmb.h
> > +++ b/include/lmb.h
> > @@ -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);
>
> Can you please add full comments in the header for new functions.
>

Sure I can but wouldn't it look odd to have one function documented in the
header but not the rest?


> > +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);
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index cd297f8202..e380a0a722 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb,
> phys_size_t size, ulong align, phy
> >         return 0;
> >  }
> >
> > +/*
> > + * Try to allocate a specific address range: must be in defined memory
> but not
> > + * reserved
> > + */
> > +phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> phys_size_t size)
> > +{
> > +       long j;
>
> How about addr instead of j? I think single-char vars are OK for loop
> counters, etc. but this is not that.
>

Sure.


> > +
> > +       /* Check if the requested address is in one of the memory
> regions */
> > +       j = lmb_overlaps_region(&lmb->memory, base, size);
> > +       if (j >= 0) {
> > +               /*
> > +                * Check if the requested end address is in the same
> memory
> > +                * region we found.
> > +                */
> > +               if (lmb_addrs_overlap(lmb->memory.region[j].base,
> > +                                     lmb->memory.region[j].size, base +
> size -
> > +                                     1, 1)) {
> > +                       /* ok, reserve the memory */
> > +                       if (lmb_reserve(lmb, base, size) >= 0)
> > +                               return base;
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +
> > +/* Return number of bytes from a given address that are free */
> > +phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)
>
> I support you use 'unreserved' instead of 'free' due to some subtle
> difference in meaning? Can you add a comment somewhere about this?
>

Actually no, the name could be changed to 'lmb_get_free_size' if you like.


> > +{
> > +       int i;
> > +       long j;
>
> Here too - addr?
>

Yes.


> > +
> > +       /* check if the requested address is in the memory regions */
> > +       j = lmb_overlaps_region(&lmb->memory, addr, 1);
> > +       if (j >= 0) {
> > +               for (i = 0; i < lmb->reserved.cnt; i++) {
> > +                       if (addr < lmb->reserved.region[i].base) {
> > +                               /* first reserved range > requested
> address */
> > +                               return lmb->reserved.region[i].base -
> addr;
> > +                       }
> > +                       if (lmb->reserved.region[i].base +
> > +                           lmb->reserved.region[i].size > addr) {
> > +                               /* requested addr is in this reserved
> range */
> > +                               return 0;
> > +                       }
> > +               }
> > +               /* if we come here: no reserved ranges above requested
> addr */
> > +               return lmb->memory.region[lmb->memory.cnt - 1].base +
> > +                      lmb->memory.region[lmb->memory.cnt - 1].size -
> addr;
> > +       }
> > +       return 0;
> > +}
> > +
>

Sigh, I'll re-spin again, but I won't find the time to do so before next
week...

Regards,
Simon

>

[U-Boot] [PATCH v10 05/10] lib: lmb: extend lmb for checks at load time

[Tom Rini]

On Wed, Jan 16, 2019 at 10:44:16PM +0100, Simon Goldschmidt wrote:
> Am Mi., 16. Jan. 2019, 22:34 hat Simon Glass <sjg@chromium.org> geschrieben:
> 
> > Hi Simon,
> >
> > On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
> > <simon.k.r.goldschmidt@gmail.com> wrote:
> > >
> > > 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 <simon.k.r.goldschmidt@gmail.com>
> > > ---
> > >
> > > Changes in v10: None
> > > Changes in v9: None
> > > Changes in v8: None
> > > Changes in v7: None
> > > Changes in v6: None
> > > 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(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But please see suggestions/nits below.
> >
> > >
> > > diff --git a/include/lmb.h b/include/lmb.h
> > > index f04d058093..7d7e2a78dc 100644
> > > --- a/include/lmb.h
> > > +++ b/include/lmb.h
> > > @@ -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);
> >
> > Can you please add full comments in the header for new functions.
> >
> 
> Sure I can but wouldn't it look odd to have one function documented in the
> header but not the rest?
> 
> 
> > > +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);
> > >
> > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > index cd297f8202..e380a0a722 100644
> > > --- a/lib/lmb.c
> > > +++ b/lib/lmb.c
> > > @@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb,
> > phys_size_t size, ulong align, phy
> > >         return 0;
> > >  }
> > >
> > > +/*
> > > + * Try to allocate a specific address range: must be in defined memory
> > but not
> > > + * reserved
> > > + */
> > > +phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> > phys_size_t size)
> > > +{
> > > +       long j;
> >
> > How about addr instead of j? I think single-char vars are OK for loop
> > counters, etc. but this is not that.
> >
> 
> Sure.
> 
> 
> > > +
> > > +       /* Check if the requested address is in one of the memory
> > regions */
> > > +       j = lmb_overlaps_region(&lmb->memory, base, size);
> > > +       if (j >= 0) {
> > > +               /*
> > > +                * Check if the requested end address is in the same
> > memory
> > > +                * region we found.
> > > +                */
> > > +               if (lmb_addrs_overlap(lmb->memory.region[j].base,
> > > +                                     lmb->memory.region[j].size, base +
> > size -
> > > +                                     1, 1)) {
> > > +                       /* ok, reserve the memory */
> > > +                       if (lmb_reserve(lmb, base, size) >= 0)
> > > +                               return base;
> > > +               }
> > > +       }
> > > +       return 0;
> > > +}
> > > +
> > > +/* Return number of bytes from a given address that are free */
> > > +phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)
> >
> > I support you use 'unreserved' instead of 'free' due to some subtle
> > difference in meaning? Can you add a comment somewhere about this?
> >
> 
> Actually no, the name could be changed to 'lmb_get_free_size' if you like.
> 
> 
> > > +{
> > > +       int i;
> > > +       long j;
> >
> > Here too - addr?
> >
> 
> Yes.
> 
> 
> > > +
> > > +       /* check if the requested address is in the memory regions */
> > > +       j = lmb_overlaps_region(&lmb->memory, addr, 1);
> > > +       if (j >= 0) {
> > > +               for (i = 0; i < lmb->reserved.cnt; i++) {
> > > +                       if (addr < lmb->reserved.region[i].base) {
> > > +                               /* first reserved range > requested
> > address */
> > > +                               return lmb->reserved.region[i].base -
> > addr;
> > > +                       }
> > > +                       if (lmb->reserved.region[i].base +
> > > +                           lmb->reserved.region[i].size > addr) {
> > > +                               /* requested addr is in this reserved
> > range */
> > > +                               return 0;
> > > +                       }
> > > +               }
> > > +               /* if we come here: no reserved ranges above requested
> > addr */
> > > +               return lmb->memory.region[lmb->memory.cnt - 1].base +
> > > +                      lmb->memory.region[lmb->memory.cnt - 1].size -
> > addr;
> > > +       }
> > > +       return 0;
> > > +}
> > > +
> >
> 
> Sigh, I'll re-spin again, but I won't find the time to do so before next
> week...

Do it as a follow-up please, I'm testing v10 atm.  Thanks!

-- 
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/20190116/cf65b368/attachment.sig>

[U-Boot] [PATCH v10 05/10] lib: lmb: extend lmb for checks at load time

[Simon Glass]

On Wed, 16 Jan 2019 at 14:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jan 16, 2019 at 10:44:16PM +0100, Simon Goldschmidt wrote:
> > Am Mi., 16. Jan. 2019, 22:34 hat Simon Glass <sjg@chromium.org> geschrieben:
> >
> > > Hi Simon,
> > >
> > > On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
> > > <simon.k.r.goldschmidt@gmail.com> wrote:
> > > >
> > > > 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 <simon.k.r.goldschmidt@gmail.com>
> > > > ---
> > > >
> > > > Changes in v10: None
> > > > Changes in v9: None
> > > > Changes in v8: None
> > > > Changes in v7: None
> > > > Changes in v6: None
> > > > 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(+)
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > But please see suggestions/nits below.
> > >
> > > >
> > > > diff --git a/include/lmb.h b/include/lmb.h
> > > > index f04d058093..7d7e2a78dc 100644
> > > > --- a/include/lmb.h
> > > > +++ b/include/lmb.h
> > > > @@ -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);
> > >
> > > Can you please add full comments in the header for new functions.
> > >
> >
> > Sure I can but wouldn't it look odd to have one function documented in the
> > header but not the rest?
> >
> >
> > > > +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);
> > > >
> > > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > > index cd297f8202..e380a0a722 100644
> > > > --- a/lib/lmb.c
> > > > +++ b/lib/lmb.c
> > > > @@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb,
> > > phys_size_t size, ulong align, phy
> > > >         return 0;
> > > >  }
> > > >
> > > > +/*
> > > > + * Try to allocate a specific address range: must be in defined memory
> > > but not
> > > > + * reserved
> > > > + */
> > > > +phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> > > phys_size_t size)
> > > > +{
> > > > +       long j;
> > >
> > > How about addr instead of j? I think single-char vars are OK for loop
> > > counters, etc. but this is not that.
> > >
> >
> > Sure.
> >
> >
> > > > +
> > > > +       /* Check if the requested address is in one of the memory
> > > regions */
> > > > +       j = lmb_overlaps_region(&lmb->memory, base, size);
> > > > +       if (j >= 0) {
> > > > +               /*
> > > > +                * Check if the requested end address is in the same
> > > memory
> > > > +                * region we found.
> > > > +                */
> > > > +               if (lmb_addrs_overlap(lmb->memory.region[j].base,
> > > > +                                     lmb->memory.region[j].size, base +
> > > size -
> > > > +                                     1, 1)) {
> > > > +                       /* ok, reserve the memory */
> > > > +                       if (lmb_reserve(lmb, base, size) >= 0)
> > > > +                               return base;
> > > > +               }
> > > > +       }
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +/* Return number of bytes from a given address that are free */
> > > > +phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)
> > >
> > > I support you use 'unreserved' instead of 'free' due to some subtle
> > > difference in meaning? Can you add a comment somewhere about this?
> > >
> >
> > Actually no, the name could be changed to 'lmb_get_free_size' if you like.
> >
> >
> > > > +{
> > > > +       int i;
> > > > +       long j;
> > >
> > > Here too - addr?
> > >
> >
> > Yes.
> >
> >
> > > > +
> > > > +       /* check if the requested address is in the memory regions */
> > > > +       j = lmb_overlaps_region(&lmb->memory, addr, 1);
> > > > +       if (j >= 0) {
> > > > +               for (i = 0; i < lmb->reserved.cnt; i++) {
> > > > +                       if (addr < lmb->reserved.region[i].base) {
> > > > +                               /* first reserved range > requested
> > > address */
> > > > +                               return lmb->reserved.region[i].base -
> > > addr;
> > > > +                       }
> > > > +                       if (lmb->reserved.region[i].base +
> > > > +                           lmb->reserved.region[i].size > addr) {
> > > > +                               /* requested addr is in this reserved
> > > range */
> > > > +                               return 0;
> > > > +                       }
> > > > +               }
> > > > +               /* if we come here: no reserved ranges above requested
> > > addr */
> > > > +               return lmb->memory.region[lmb->memory.cnt - 1].base +
> > > > +                      lmb->memory.region[lmb->memory.cnt - 1].size -
> > > addr;
> > > > +       }
> > > > +       return 0;
> > > > +}
> > > > +
> > >
> >
> > Sigh, I'll re-spin again, but I won't find the time to do so before next
> > week...
>
> Do it as a follow-up please, I'm testing v10 atm.  Thanks!

Yes, these are all minor points and you have my review tag, thank you.

Regards,
Simon

[U-Boot] [U-Boot,v10,01/10] test: add test for lib/lmb.c

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:14PM +0100, Simon Goldschmidt wrote:

> Add basic tests for the lmb memory allocation code used to reserve and
> allocate memory during boot.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
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/20190117/5e2c3e18/attachment.sig>

[U-Boot] [U-Boot, v10, 02/10] lmb: fix allocation at end of address range

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:15PM +0100, Simon Goldschmidt wrote:

> 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 <simon.k.r.goldschmidt@gmail.com>

Applied to u-boot/master, thanks!

-- 
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/20190117/44c1cb6c/attachment.sig>

[U-Boot] [U-Boot, v10, 03/10] lib: lmb: reserving overlapping regions should fail

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:16PM +0100, Simon Goldschmidt wrote:

> 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.
> 
> Also, to keep reserved memory correct after 'free', reserved entries
> created by allocating memory must not set their size to a multiple
> of alignment but to the original size. This ensures the reserved
> region is completely removed when the caller calls 'lmb_free', as
> this one takes the same size as passed to 'lmb_alloc' etc.
> 
> Add test to assert this.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Applied to u-boot/master, thanks!

-- 
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/20190117/1d91310f/attachment.sig>

[U-Boot] [U-Boot, v10, 04/10] fdt: parse "reserved-memory" for memory reservation

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:17PM +0100, Simon Goldschmidt wrote:

> 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 <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
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/20190117/0687762b/attachment.sig>

[U-Boot] [U-Boot, v10, 05/10] lib: lmb: extend lmb for checks at load time

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:18PM +0100, Simon Goldschmidt wrote:

> 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 <simon.k.r.goldschmidt@gmail.com>

Applied to u-boot/master, thanks!

-- 
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/20190117/38b08694/attachment.sig>

[U-Boot] [U-Boot, v10, 06/10] fs: prevent overwriting reserved memory

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:19PM +0100, Simon Goldschmidt wrote:

> 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 <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
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/20190117/0bb565ce/attachment.sig>

[U-Boot] [U-Boot, v10, 07/10] bootm: use new common function lmb_init_and_reserve

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:20PM +0100, Simon Goldschmidt wrote:

> This reduces duplicate code only.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
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/20190117/f61f2fdd/attachment.sig>

[U-Boot] [U-Boot, v10, 08/10] lmb: remove unused extern declaration

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:21PM +0100, Simon Goldschmidt wrote:

> lmb.h includes an extern declaration of "struct lmb lmb;" which
> is not used anywhere, so remove it.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Applied to u-boot/master, thanks!

-- 
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/20190117/a76d353e/attachment.sig>

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:22PM +0100, Simon Goldschmidt wrote:

> 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 <simon.k.r.goldschmidt@gmail.com>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>

With some lib/Makefile tweaks for the odd SPL+network use cases:
Applied to u-boot/master, thanks!

-- 
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/20190117/0a003957/attachment.sig>

[U-Boot] [U-Boot, v10, 10/10] arm: bootm: fix sp detection at end of address range

[Tom Rini]

On Mon, Jan 14, 2019 at 10:38:23PM +0100, Simon Goldschmidt wrote:

> This fixes  'arch_lmb_reserve()' for ARM that tries to detect in which
> DRAM bank 'sp' is in.
> 
> This code failed if a bank was at the end of physical address range
> (i.e. size + length overflowed to 0).
> 
> To fix this, calculate 'bank_end' as 'size + length - 1' so that such
> banks end at 0xffffffff, not 0.
> 
> Fixes: 15751403b6 ("ARM: bootm: don't assume sp is in DRAM bank 0")
> Reported-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

-- 
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/20190117/83556f5a/attachment.sig>

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Heinrich Schuchardt]

TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
> 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 <simon.k.r.goldschmidt@gmail.com>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> ---

Hello Simon,

due to this patch merged as a156c47e39ad7d00 on
vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
was working in v2019.01

Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.

I put in an extra printf() and got:
TFTP error: trying to overwrite reserved memory...
storeaddr 0, tftp_load_addr 0, tftp_load_size 0

It is not even possible to disable the checks by undefining CONFIG_LMB
because a compile error arises without CONFIG_LMB:

cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’}
has no member named ‘lmb’

I think the code should compile if CONFIG_LMB is undefined.

Further for all boards 'dhcp filename' should be working after your
patch series if it was working before the patch series.

Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard
coded CONFIG symbols? Consider moving it to Kconfig.

The logic you use in tftp_init_load_addr() is problematic:

Essentially it allows loading via tftp only in a single region within
the first DRAM bank. Why shouldn't I load to the second DRAM bank?

Even in a single DRAM bank we will have several reserved regions and in
between them several allowable regions for loading.

The LMB tests do not even find all reserved regions. E.g. on x86_64 it
allows loading to 0x1000000 though this address is used as a reserved
region for PCI, loading to which leads to a crash.

@Tom
This LMB patch series stops us from straightening out the Python tests
for tftp to make efi-next build without Travis CI error. Please, advise
how to proceed.

Best regards

Heinrich

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Simon Goldschmidt]

Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt:
> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
>> 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 <simon.k.r.goldschmidt@gmail.com>
>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
> 
> Hello Simon,
> 
> due to this patch merged as a156c47e39ad7d00 on
> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
> was working in v2019.01
> 
> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.

OK, that's probably not expected ;-)

I'd appreciate it if you could continue to track this down to get it fixed.

> 
> I put in an extra printf() and got:
> TFTP error: trying to overwrite reserved memory...
> storeaddr 0, tftp_load_addr 0, tftp_load_size 0

I don't know the first. The latter 2 are not initialized yet in this 
error path and so are expected to be zero here.

Could you run that test again if I sent you a patch enabling required 
output for me to debug this?

> 
> It is not even possible to disable the checks by undefining CONFIG_LMB
> because a compile error arises without CONFIG_LMB:
> 
> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’}
> has no member named ‘lmb’
> 
> I think the code should compile if CONFIG_LMB is undefined.

You're right, it should compile without CONFIG_LMB. It did initially, so 
I guess that got lost somewhere during all the versions until v10, 
sorry. I'll work on that.

> 
> Further for all boards 'dhcp filename' should be working after your
> patch series if it was working before the patch series.

Well, I wouldn't say it like that. This new code is required from a 
security point of view. There might be boards violating these 
requirements, I can't tell. But it's true that until your ${loadaddr} is 
not completely bogus, 'dhcp filename' should continue to work, yes. If 
not, let's work on this.

> 
> Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard
> coded CONFIG symbols? Consider moving it to Kconfig.

Ehrm, sorry, I can't follow you. Which new config symbols are you 
talking about? CONFIG_LMB in ARM's config.h is more than 8 years old!

> 
> The logic you use in tftp_init_load_addr() is problematic:
> 
> Essentially it allows loading via tftp only in a single region within
> the first DRAM bank. Why shouldn't I load to the second DRAM bank?
> 
> Even in a single DRAM bank we will have several reserved regions and in
> between them several allowable regions for loading.

What leads you to think it's only a single region? Multiple reserved 
regions should work and the 'holes' in between should be valid tftp 
targets. This is tested in the unit tests.

You're right that currently only the first DRAM bank works. Let me work 
on that...

> 
> The LMB tests do not even find all reserved regions. E.g. on x86_64 it
> allows loading to 0x1000000 though this address is used as a reserved
> region for PCI, loading to which leads to a crash.

LMB is a long established concept for U-Boot loading boot files. I added 
using it to the 'load' and 'tftp' commands. If x86_64 fails to reserve 
memory for LMB, I think x86_64 should be fixed to do so (e.g. via 
'arch_lmb_reserve').

> 
> @Tom
> This LMB patch series stops us from straightening out the Python tests
> for tftp to make efi-next build without Travis CI error. Please, advise
> how to proceed.

My idea of how to proceed would be: let's just sort out these issues as 
fast as possible. I'll send you a patch to debug why tftp thinks it 
would overwrite reserved memory. Also, I'll fix the compile error with 
CONFIG_LMB disabled and I'll try to add DRAM banks other than the first.

Regards,
Simon

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Heinrich Schuchardt]

On 1/26/19 9:46 AM, Simon Goldschmidt wrote:
> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt:
>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
>>> 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 <simon.k.r.goldschmidt@gmail.com>
>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>
>> Hello Simon,
>>
>> due to this patch merged as a156c47e39ad7d00 on
>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
>> was working in v2019.01
>>
>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.
> 
> OK, that's probably not expected ;-)
> 
> I'd appreciate it if you could continue to track this down to get it fixed.

Let's see how far I get.

You can easily test yourself with QEMU. I was using:

QEMU_AUDIO_DRV=none qemu-system-arm \
-M vexpress-a15 -cpu cortex-a15 -kernel u-boot \
-netdev \
user,id=net0,tftp=tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \
-net nic,model=lan9118,netdev=net0 \
-m 1024M --nographic \
-drive if=sd,file=img.vexpress,media=disk,format=raw

> 
>>
>> I put in an extra printf() and got:
>> TFTP error: trying to overwrite reserved memory...
>> storeaddr 0, tftp_load_addr 0, tftp_load_size 0
> 
> I don't know the first. The latter 2 are not initialized yet in this
> error path and so are expected to be zero here.
> 
> Could you run that test again if I sent you a patch enabling required
> output for me to debug this?

Sure.

> 
>>
>> It is not even possible to disable the checks by undefining CONFIG_LMB
>> because a compile error arises without CONFIG_LMB:
>>
>> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’}
>> has no member named ‘lmb’
>>
>> I think the code should compile if CONFIG_LMB is undefined.
> 
> You're right, it should compile without CONFIG_LMB. It did initially, so
> I guess that got lost somewhere during all the versions until v10,
> sorry. I'll work on that.
> 
>>
>> Further for all boards 'dhcp filename' should be working after your
>> patch series if it was working before the patch series.
> 
> Well, I wouldn't say it like that. This new code is required from a
> security point of view. There might be boards violating these
> requirements, I can't tell. But it's true that until your ${loadaddr} is
> not completely bogus, 'dhcp filename' should continue to work, yes. If
> not, let's work on this.

I think we are on the same line.

> 
>>
>> Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard
>> coded CONFIG symbols? Consider moving it to Kconfig.
> 
> Ehrm, sorry, I can't follow you. Which new config symbols are you
> talking about? CONFIG_LMB in ARM's config.h is more than 8 years old!

Sorry, I did not check this. So you didn't put in a new switch.

> 
>>
>> The logic you use in tftp_init_load_addr() is problematic:
>>
>> Essentially it allows loading via tftp only in a single region within
>> the first DRAM bank. Why shouldn't I load to the second DRAM bank?
>>
>> Even in a single DRAM bank we will have several reserved regions and in
>> between them several allowable regions for loading.
> 
> What leads you to think it's only a single region? Multiple reserved
> regions should work and the 'holes' in between should be valid tftp
> targets. This is tested in the unit tests.

I did not see that load_addr is a global set in cmd/net.c based on the
parameter passed to the tftp command.

> 
> You're right that currently only the first DRAM bank works. Let me work
> on that...
> 
>>
>> The LMB tests do not even find all reserved regions. E.g. on x86_64 it
>> allows loading to 0x1000000 though this address is used as a reserved
>> region for PCI, loading to which leads to a crash.
> 
> LMB is a long established concept for U-Boot loading boot files. I added
> using it to the 'load' and 'tftp' commands. If x86_64 fails to reserve
> memory for LMB, I think x86_64 should be fixed to do so (e.g. via
> 'arch_lmb_reserve').
> 
>>
>> @Tom
>> This LMB patch series stops us from straightening out the Python tests
>> for tftp to make efi-next build without Travis CI error. Please, advise
>> how to proceed.
> 
> My idea of how to proceed would be: let's just sort out these issues as
> fast as possible. I'll send you a patch to debug why tftp thinks it
> would overwrite reserved memory. Also, I'll fix the compile error with
> CONFIG_LMB disabled and I'll try to add DRAM banks other than the first.
> 
> Regards,
> Simon
>

Best regards

Heinrich

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Tom Rini]

On Sat, Jan 26, 2019 at 09:46:35AM +0100, Simon Goldschmidt wrote:
> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt:
> >TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
> >>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 <simon.k.r.goldschmidt@gmail.com>
> >>Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> >>---
> >
> >Hello Simon,
> >
> >due to this patch merged as a156c47e39ad7d00 on
> >vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
> >was working in v2019.01
> >
> >Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.
> 
> OK, that's probably not expected ;-)
> 
> I'd appreciate it if you could continue to track this down to get it fixed.
> 
> >
> >I put in an extra printf() and got:
> >TFTP error: trying to overwrite reserved memory...
> >storeaddr 0, tftp_load_addr 0, tftp_load_size 0
> 
> I don't know the first. The latter 2 are not initialized yet in this error
> path and so are expected to be zero here.
> 
> Could you run that test again if I sent you a patch enabling required output
> for me to debug this?
> 
> >
> >It is not even possible to disable the checks by undefining CONFIG_LMB
> >because a compile error arises without CONFIG_LMB:
> >
> >cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’}
> >has no member named ‘lmb’
> >
> >I think the code should compile if CONFIG_LMB is undefined.
> 
> You're right, it should compile without CONFIG_LMB. It did initially, so I
> guess that got lost somewhere during all the versions until v10, sorry. I'll
> work on that.

That might be on me.  There were a few cases in the networking code
where the patch broke building the existing world.

-- 
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/20190126/af03687d/attachment.sig>

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Heinrich Schuchardt]

On 1/26/19 10:56 AM, Heinrich Schuchardt wrote:
> On 1/26/19 9:46 AM, Simon Goldschmidt wrote:
>> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt:
>>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
>>>> 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 <simon.k.r.goldschmidt@gmail.com>
>>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>>> ---
>>>
>>> Hello Simon,
>>>
>>> due to this patch merged as a156c47e39ad7d00 on
>>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
>>> was working in v2019.01
>>>
>>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.
>>
>> OK, that's probably not expected ;-)
>>
>> I'd appreciate it if you could continue to track this down to get it fixed.
> 
> Let's see how far I get.

bdinfo shows:

DRAM bank   = 0x00000000
-> start    = 0x80000000
-> size     = 0x20000000
DRAM bank   = 0x00000001
-> start    = 0xa0000000
-> size     = 0x20000000

printenv:
loadaddr=0xa0008000

So the load address is in the second DRAM bank.

I guess we need changes in the following places:

t/tftp.c:609: lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start,
fs/fs.c:456:    lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start,
common/bootm.c:62:      lmb_init_and_reserve(&images->lmb,
(phys_addr_t)mem_start, mem_size,

I wonder why bootm.c is different and why isn't the fdt considered?

I would suggest the following:

Remove parameter lmb from lmb_get_unreserved_size(). Instead let
lmb_get_unreserved_size() check if a static struct lmb in lib/lmb.c is
initialized. If not use the different DRAM banks and the fdt for
initialization.

Best regards

Heinrich

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Simon Goldschmidt]

Am 26.01.2019 um 14:17 schrieb Tom Rini:
> On Sat, Jan 26, 2019 at 09:46:35AM +0100, Simon Goldschmidt wrote:
>> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt:
>>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
>>>> 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 <simon.k.r.goldschmidt@gmail.com>
>>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>>> ---
>>>
>>> Hello Simon,
>>>
>>> due to this patch merged as a156c47e39ad7d00 on
>>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
>>> was working in v2019.01
>>>
>>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.
>>
>> OK, that's probably not expected ;-)
>>
>> I'd appreciate it if you could continue to track this down to get it fixed.
>>
>>>
>>> I put in an extra printf() and got:
>>> TFTP error: trying to overwrite reserved memory...
>>> storeaddr 0, tftp_load_addr 0, tftp_load_size 0
>>
>> I don't know the first. The latter 2 are not initialized yet in this error
>> path and so are expected to be zero here.
>>
>> Could you run that test again if I sent you a patch enabling required output
>> for me to debug this?
>>
>>>
>>> It is not even possible to disable the checks by undefining CONFIG_LMB
>>> because a compile error arises without CONFIG_LMB:
>>>
>>> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’}
>>> has no member named ‘lmb’
>>>
>>> I think the code should compile if CONFIG_LMB is undefined.
>>
>> You're right, it should compile without CONFIG_LMB. It did initially, so I
>> guess that got lost somewhere during all the versions until v10, sorry. I'll
>> work on that.
> 
> That might be on me.  There were a few cases in the networking code
> where the patch broke building the existing world.

Trying again to compile with CONFIG_LMB disabled, it didn't work at all. 
It failed in places none of us touched for about 8 years, so I don't 
think it was you.

OTOH, I don't know what I had been testing to think it works with 
CONFIG_LMB disabled. I had to disable quite a few commands and features 
to keep it compiling.

In the end, I think we'll have to decide if we want to make it work with 
CONFIG_LMB disabled or if we make this mandatory.

What I did see is that some of the architectures don't overwrite 
'arch_lmb_reserve' and are thus probably still affected by these CVEs...

Regards,
Simon

[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[Simon Goldschmidt]

Am 26.01.2019 um 10:56 schrieb Heinrich Schuchardt:
> On 1/26/19 9:46 AM, Simon Goldschmidt wrote:
>> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt:
>>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote:
>>>> 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 <simon.k.r.goldschmidt@gmail.com>
>>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>>> ---
>>>
>>> Hello Simon,
>>>
>>> due to this patch merged as a156c47e39ad7d00 on
>>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It
>>> was working in v2019.01
>>>
>>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig.
>>
>> OK, that's probably not expected ;-)
>>
>> I'd appreciate it if you could continue to track this down to get it fixed.
> 
> Let's see how far I get.
> 
> You can easily test yourself with QEMU. I was using:
> 
> QEMU_AUDIO_DRV=none qemu-system-arm \
> -M vexpress-a15 -cpu cortex-a15 -kernel u-boot \
> -netdev \
> user,id=net0,tftp=tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \
> -net nic,model=lan9118,netdev=net0 \
> -m 1024M --nographic \
> -drive if=sd,file=img.vexpress,media=disk,format=raw

Yes, this worked quite well (after creating the 'img.vexpress' file, 
that is), and 'dhcp somefile' now works for me in that configuration. 
Thanks for the hint.

> 
>>
>>>
>>> I put in an extra printf() and got:
>>> TFTP error: trying to overwrite reserved memory...
>>> storeaddr 0, tftp_load_addr 0, tftp_load_size 0
>>
>> I don't know the first. The latter 2 are not initialized yet in this
>> error path and so are expected to be zero here.
>>
>> Could you run that test again if I sent you a patch enabling required
>> output for me to debug this?
> 
> Sure.
> 
>>
>>>
>>> It is not even possible to disable the checks by undefining CONFIG_LMB
>>> because a compile error arises without CONFIG_LMB:
>>>
>>> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’}
>>> has no member named ‘lmb’
>>>
>>> I think the code should compile if CONFIG_LMB is undefined.
>>
>> You're right, it should compile without CONFIG_LMB. It did initially, so
>> I guess that got lost somewhere during all the versions until v10,
>> sorry. I'll work on that.
>>
>>>
>>> Further for all boards 'dhcp filename' should be working after your
>>> patch series if it was working before the patch series.
>>
>> Well, I wouldn't say it like that. This new code is required from a
>> security point of view. There might be boards violating these
>> requirements, I can't tell. But it's true that until your ${loadaddr} is
>> not completely bogus, 'dhcp filename' should continue to work, yes. If
>> not, let's work on this.
> 
> I think we are on the same line.
> 
>>
>>>
>>> Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard
>>> coded CONFIG symbols? Consider moving it to Kconfig.
>>
>> Ehrm, sorry, I can't follow you. Which new config symbols are you
>> talking about? CONFIG_LMB in ARM's config.h is more than 8 years old!
> 
> Sorry, I did not check this. So you didn't put in a new switch.
> 
>>
>>>
>>> The logic you use in tftp_init_load_addr() is problematic:
>>>
>>> Essentially it allows loading via tftp only in a single region within
>>> the first DRAM bank. Why shouldn't I load to the second DRAM bank?
>>>
>>> Even in a single DRAM bank we will have several reserved regions and in
>>> between them several allowable regions for loading.
>>
>> What leads you to think it's only a single region? Multiple reserved
>> regions should work and the 'holes' in between should be valid tftp
>> targets. This is tested in the unit tests.
> 
> I did not see that load_addr is a global set in cmd/net.c based on the
> parameter passed to the tftp command.
> 
>>
>> You're right that currently only the first DRAM bank works. Let me work
>> on that...
>>
>>>
>>> The LMB tests do not even find all reserved regions. E.g. on x86_64 it
>>> allows loading to 0x1000000 though this address is used as a reserved
>>> region for PCI, loading to which leads to a crash.
>>
>> LMB is a long established concept for U-Boot loading boot files. I added
>> using it to the 'load' and 'tftp' commands. If x86_64 fails to reserve
>> memory for LMB, I think x86_64 should be fixed to do so (e.g. via
>> 'arch_lmb_reserve').
>>
>>>
>>> @Tom
>>> This LMB patch series stops us from straightening out the Python tests
>>> for tftp to make efi-next build without Travis CI error. Please, advise
>>> how to proceed.
>>
>> My idea of how to proceed would be: let's just sort out these issues as
>> fast as possible. I'll send you a patch to debug why tftp thinks it
>> would overwrite reserved memory. Also, I'll fix the compile error with
>> CONFIG_LMB disabled and I'll try to add DRAM banks other than the first.

So I just sent a patch that should fix the "multiple DRAM banks" issue. 
I gave up compiling without CONFIG_LMB for now, I guess we need a more 
global decision on if we want that or not, since those compiler errors 
seem to be a reuslt of changes much more in the past than I thought...

I hope this new patch fixes things for you. Thanks for working on this 
with me!

Regards,
Simon

[U-Boot] [U-Boot, v10, 04/10] fdt: parse "reserved-memory" for memory reservation

[Eugeniu Rosca]

Simon, Marek, All,

Booting Linux on H3-ES2.0-Salvator-X, this patch contributes with
below runtime errors:

ERROR: reserving fdt memory region failed (addr=54000000 size=3000000)
ERROR: reserving fdt memory region failed (addr=57000000 size=1000000)
ERROR: reserving fdt memory region failed (addr=58000000 size=18000000)
ERROR: reserving fdt memory region failed (addr=70000000 size=10000000)

I use rcar-3.9.2 device trees from:
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts?h=rcar-3.9.2

The mainline v5.0 device trees lack the reserved-memory node, so I
expect the errors will not show up with vanilla kernel and DTB.
FTR, the errors appear regardless of the value for U-Boot
CONFIG_ARCH_FIXUP_FDT_MEMORY (=n is the default).
I tried to play with different values of bootm_size (including the
recent https://patchwork.ozlabs.org/patch/1052012/), with no success.

I would appreciate your view on how to tackle this. TIA!

Best regards,
Eugeniu.

[U-Boot] [U-Boot, v10, 04/10] fdt: parse "reserved-memory" for memory reservation

[Marek Vasut]

On 3/6/19 12:26 AM, Eugeniu Rosca wrote:
> Simon, Marek, All,

Hi,

> Booting Linux on H3-ES2.0-Salvator-X, this patch contributes with
> below runtime errors:
> 
> ERROR: reserving fdt memory region failed (addr=54000000 size=3000000)
> ERROR: reserving fdt memory region failed (addr=57000000 size=1000000)
> ERROR: reserving fdt memory region failed (addr=58000000 size=18000000)
> ERROR: reserving fdt memory region failed (addr=70000000 size=10000000)
> 
> I use rcar-3.9.2 device trees from:
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts?h=rcar-3.9.2

BSP DTs are not supported, we're moving to Linux 5.0 DTs in the next
release, I'll be posting patches once they receive proper testing. The
patches are here [1], along M3N ULCB support, which I believe is broken
thus far and so none of that is posted to the ML yet. I'll be fixing
that at the end of month, once I have physical access to the board (and
there's more stuff coming :) )

[1] https://github.com/marex/u-boot-sh/tree/m3nulcb-v1

> The mainline v5.0 device trees lack the reserved-memory node, so I
> expect the errors will not show up with vanilla kernel and DTB.
> FTR, the errors appear regardless of the value for U-Boot
> CONFIG_ARCH_FIXUP_FDT_MEMORY (=n is the default).
> I tried to play with different values of bootm_size (including the
> recent https://patchwork.ozlabs.org/patch/1052012/), with no success.

The reserved memory nodes are used for the FCNL, right ?
If so, then we will have to deal with it somehow. Can you debug the
problem and propose a patch ?

> I would appreciate your view on how to tackle this. TIA!
> 
> Best regards,
> Eugeniu.
> 


-- 
Best regards,
Marek Vasut