* [U-Boot] [PATCH v6 1/9] test: add test for lib/lmb.c
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2019-01-05 1:56 ` Simon Glass
2018-12-14 20:13 ` [U-Boot] [PATCH v6 2/9] lmb: fix allocation at end of address range Simon Goldschmidt
` (7 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 1/9] test: add test for lib/lmb.c
2018-12-14 20:13 ` [U-Boot] [PATCH v6 1/9] test: add test for lib/lmb.c Simon Goldschmidt
@ 2019-01-05 1:56 ` Simon Glass
2019-01-14 19:19 ` Simon Goldschmidt
0 siblings, 1 reply; 17+ messages in thread
From: Simon Glass @ 2019-01-05 1:56 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Fri, 14 Dec 2018 at 13:14, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> 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>
> ---
>
> 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
Reviewed-by: Simon Glass <sjg@chromium.org>
Seems fine. I wonder if it would be easier to simulate a 10-byte
memory size? It shouldn't matter how big it is.
Regards,
Simon
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v6 1/9] test: add test for lib/lmb.c
2019-01-05 1:56 ` Simon Glass
@ 2019-01-14 19:19 ` Simon Goldschmidt
0 siblings, 0 replies; 17+ messages in thread
From: Simon Goldschmidt @ 2019-01-14 19:19 UTC (permalink / raw)
To: u-boot
Am 05.01.2019 um 02:56 schrieb Simon Glass:
> Hi Simon,
>
> On Fri, 14 Dec 2018 at 13:14, Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> 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>
>> ---
>>
>> 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
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Seems fine. I wonder if it would be easier to simulate a 10-byte
> memory size? It shouldn't matter how big it is.
This one slipped through somehow, sorry.
I wrote the test by simulating real values. Since there is no memory
involved, only numbers, personally, I don't think simulating 10 bytes
makes it easier than simulating 512 MiB. I'd leave it like it is, unless
you insist...
Regards,
Simon
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v6 2/9] lmb: fix allocation at end of address range
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 1/9] test: add test for lib/lmb.c Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 3/9] lib: lmb: reserving overlapping regions should fail Simon Goldschmidt
` (6 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 3/9] lib: lmb: reserving overlapping regions should fail
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 1/9] test: add test for lib/lmb.c Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 2/9] lmb: fix allocation at end of address range Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2018-12-15 15:10 ` Tom Rini
2018-12-14 20:13 ` [U-Boot] [PATCH v6 4/9] fdt: parse "reserved-memory" for memory reservation Simon Goldschmidt
` (5 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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 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 | 94 +++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 90 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..ffa3d53bf1 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,42 @@ 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 +290,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 +357,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 3/9] lib: lmb: reserving overlapping regions should fail
2018-12-14 20:13 ` [U-Boot] [PATCH v6 3/9] lib: lmb: reserving overlapping regions should fail Simon Goldschmidt
@ 2018-12-15 15:10 ` Tom Rini
0 siblings, 0 replies; 17+ messages in thread
From: Tom Rini @ 2018-12-15 15:10 UTC (permalink / raw)
To: u-boot
On Fri, Dec 14, 2018 at 09:13:51PM +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>
[snip]
> + 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);
I've fixed this inline for testing but, we need braces with this if/else
as I guess due to all of the macro expansion it's more than one line and
both gcc and clang fail.
--
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/20181215/dd46105f/attachment.sig>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v6 4/9] fdt: parse "reserved-memory" for memory reservation
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
` (2 preceding siblings ...)
2018-12-14 20:13 ` [U-Boot] [PATCH v6 3/9] lib: lmb: reserving overlapping regions should fail Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2019-01-05 1:56 ` Simon Glass
2018-12-14 20:13 ` [U-Boot] [PATCH v6 5/9] lib: lmb: extend lmb for checks at load time Simon Goldschmidt
` (4 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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>
---
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..1972fe8cf1 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_FIT) += fdtdec.o
obj-$(CONFIG_FIT) += fdtdec_common.o
obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 4/9] fdt: parse "reserved-memory" for memory reservation
2018-12-14 20:13 ` [U-Boot] [PATCH v6 4/9] fdt: parse "reserved-memory" for memory reservation Simon Goldschmidt
@ 2019-01-05 1:56 ` Simon Glass
0 siblings, 0 replies; 17+ messages in thread
From: Simon Glass @ 2019-01-05 1:56 UTC (permalink / raw)
To: u-boot
On Fri, 14 Dec 2018 at 13:14, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> 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>
> ---
>
> 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(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v6 5/9] lib: lmb: extend lmb for checks at load time
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
` (3 preceding siblings ...)
2018-12-14 20:13 ` [U-Boot] [PATCH v6 4/9] fdt: parse "reserved-memory" for memory reservation Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 6/9] fs: prevent overwriting reserved memory Simon Goldschmidt
` (3 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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 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 ffa3d53bf1..f6f4b76dc0 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -396,3 +396,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 6/9] fs: prevent overwriting reserved memory
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
` (4 preceding siblings ...)
2018-12-14 20:13 ` [U-Boot] [PATCH v6 5/9] lib: lmb: extend lmb for checks at load time Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2019-01-05 1:56 ` Simon Glass
2018-12-14 20:13 ` [U-Boot] [PATCH v6 7/9] bootm: use new common function lmb_init_and_reserve Simon Goldschmidt
` (2 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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>
---
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..400aa921a7 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 -1;
+}
+#endif
+
+static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
+ int do_lmb_check, loff_t *actread)
{
struct fstype_info *info = fs_get_info(fs_type);
void *buf;
int ret;
+#ifdef CONFIG_LMB
+ if (do_lmb_check) {
+ ret = fs_read_lmb_check(filename, addr, offset, len, info);
+ if (ret)
+ return ret;
+ }
+#endif
+
/*
* We don't actually know how many bytes are being read, since len==0
* means read the whole file.
@@ -452,6 +496,12 @@ int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
return ret;
}
+int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
+ loff_t *actread)
+{
+ return _fs_read(filename, addr, offset, len, 0, actread);
+}
+
int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
loff_t *actwrite)
{
@@ -622,7 +672,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
pos = 0;
time = get_timer(0);
- ret = fs_read(filename, addr, pos, bytes, &len_read);
+ ret = _fs_read(filename, addr, pos, bytes, 1, &len_read);
time = get_timer(time);
if (ret < 0)
return 1;
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 6/9] fs: prevent overwriting reserved memory
2018-12-14 20:13 ` [U-Boot] [PATCH v6 6/9] fs: prevent overwriting reserved memory Simon Goldschmidt
@ 2019-01-05 1:56 ` Simon Glass
2019-01-14 15:15 ` Simon Goldschmidt
0 siblings, 1 reply; 17+ messages in thread
From: Simon Glass @ 2019-01-05 1:56 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Fri, 14 Dec 2018 at 13:14, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> 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>
> ---
>
> 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(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
How about -ENOSPC instead of -1?
Regards,
Simon
^ permalink raw reply [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 6/9] fs: prevent overwriting reserved memory
2019-01-05 1:56 ` Simon Glass
@ 2019-01-14 15:15 ` Simon Goldschmidt
0 siblings, 0 replies; 17+ messages in thread
From: Simon Goldschmidt @ 2019-01-14 15:15 UTC (permalink / raw)
To: u-boot
On Sat, Jan 5, 2019 at 2:56 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> On Fri, 14 Dec 2018 at 13:14, Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> 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>
> > ---
> >
> > 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(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> How about -ENOSPC instead of -1?
You mean in fs_read_lmb_check()? That would probably a good idea.
Not that you were replying to an old version, I had sent out v9 on 12/19/2018.
There's still -1 in there however. I'll send a v10 that fixes this.
Regards,
Simon
^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH v6 7/9] bootm: use new common function lmb_init_and_reserve
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
` (5 preceding siblings ...)
2018-12-14 20:13 ` [U-Boot] [PATCH v6 6/9] fs: prevent overwriting reserved memory Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2019-01-05 1:56 ` Simon Glass
2018-12-14 20:13 ` [U-Boot] [PATCH v6 8/9] lmb: remove unused extern declaration Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 9/9] tftp: prevent overwriting reserved memory Simon Goldschmidt
8 siblings, 1 reply; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
This reduces duplicate code only.
Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 8/9] lmb: remove unused extern declaration
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
` (6 preceding siblings ...)
2018-12-14 20:13 ` [U-Boot] [PATCH v6 7/9] bootm: use new common function lmb_init_and_reserve Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
2018-12-14 20:13 ` [U-Boot] [PATCH v6 9/9] tftp: prevent overwriting reserved memory Simon Goldschmidt
8 siblings, 0 replies; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [U-Boot] [PATCH v6 9/9] tftp: prevent overwriting reserved memory
2018-12-14 20:13 [U-Boot] [PATCH v6 0/9] Fix CVE-2018-18440 and CVE-2018-18439 Simon Goldschmidt
` (7 preceding siblings ...)
2018-12-14 20:13 ` [U-Boot] [PATCH v6 8/9] lmb: remove unused extern declaration Simon Goldschmidt
@ 2018-12-14 20:13 ` Simon Goldschmidt
8 siblings, 0 replies; 17+ messages in thread
From: Simon Goldschmidt @ 2018-12-14 20:13 UTC (permalink / raw)
To: u-boot
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>
---
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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 57 insertions(+), 9 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c
index 68ffd81414..d31364166e 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,8 @@ static ulong tftp_block_wrap;
/* memory offset due to wrapping */
static ulong tftp_block_wrap_offset;
static int tftp_state;
+static ulong tftp_load_addr;
+static ulong tftp_load_size;
#ifdef CONFIG_TFTP_TSIZE
/* The file size reported by the server */
static int tftp_tsize;
@@ -164,10 +168,11 @@ static void mcast_cleanup(void)
#endif /* CONFIG_MCAST_TFTP */
-static inline void store_block(int block, uchar *src, unsigned len)
+static inline int store_block(int block, uchar *src, unsigned int len)
{
ulong offset = block * tftp_block_size + tftp_block_wrap_offset;
ulong newsize = offset + len;
+ ulong store_addr = tftp_load_addr + offset;
#ifdef CONFIG_SYS_DIRECT_FLASH_TFTP
int i, rc = 0;
@@ -175,24 +180,30 @@ static inline void store_block(int block, uchar *src, unsigned len)
/* start address in flash? */
if (flash_info[i].flash_id == FLASH_UNKNOWN)
continue;
- if (load_addr + offset >= flash_info[i].start[0]) {
+ if (store_addr >= flash_info[i].start[0]) {
rc = 1;
break;
}
}
if (rc) { /* Flash is destination for this packet */
- rc = flash_write((char *)src, (ulong)(load_addr+offset), len);
+ rc = flash_write((char *)src, store_addr, len);
if (rc) {
flash_perror(rc);
- net_set_state(NETLOOP_FAIL);
- return;
+ return rc;
}
} else
#endif /* CONFIG_SYS_DIRECT_FLASH_TFTP */
{
- void *ptr = map_sysmem(load_addr + offset, len);
+ void *ptr;
+ if (store_addr < tftp_load_addr ||
+ store_addr + len > tftp_load_addr + tftp_load_size) {
+ puts("\nTFTP error: ");
+ puts("trying to overwrite reserved memory...\n");
+ return -1;
+ }
+ ptr = map_sysmem(store_addr, len);
memcpy(ptr, src, len);
unmap_sysmem(ptr);
}
@@ -203,6 +214,8 @@ static inline void store_block(int block, uchar *src, unsigned len)
if (net_boot_file_size < newsize)
net_boot_file_size = newsize;
+
+ return 0;
}
/* Clear our state ready for a new transfer */
@@ -606,7 +619,11 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
timeout_count_max = tftp_timeout_count_max;
net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
- store_block(tftp_cur_block - 1, pkt + 2, len);
+ if (store_block(tftp_cur_block - 1, pkt + 2, len)) {
+ eth_halt();
+ net_set_state(NETLOOP_FAIL);
+ break;
+ }
/*
* Acknowledge the block just received, which will prompt
@@ -695,6 +712,24 @@ static void tftp_timeout_handler(void)
}
}
+/* Initialize tftp_load_addr and tftp_load_size from load_addr and lmb */
+static int tftp_init_load_addr(void)
+{
+ struct lmb lmb;
+ phys_size_t max_size;
+
+ tftp_load_addr = load_addr;
+
+ lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start,
+ gd->bd->bi_dram[0].size, (void *)gd->fdt_blob);
+
+ max_size = lmb_get_unreserved_size(&lmb, tftp_load_addr);
+ if (!max_size)
+ return -1;
+
+ tftp_load_size = max_size;
+ return 0;
+}
void tftp_start(enum proto_t protocol)
{
@@ -791,7 +826,14 @@ void tftp_start(enum proto_t protocol)
} else
#endif
{
- printf("Load address: 0x%lx\n", load_addr);
+ if (tftp_init_load_addr()) {
+ eth_halt();
+ net_set_state(NETLOOP_FAIL);
+ puts("\nTFTP error: ");
+ puts("trying to overwrite reserved memory...\n");
+ return;
+ }
+ printf("Load address: 0x%lx\n", tftp_load_addr);
puts("Loading: *\b");
tftp_state = STATE_SEND_RRQ;
#ifdef CONFIG_CMD_BOOTEFI
@@ -842,9 +884,15 @@ void tftp_start_server(void)
{
tftp_filename[0] = 0;
+ if (tftp_init_load_addr()) {
+ eth_halt();
+ net_set_state(NETLOOP_FAIL);
+ puts("\nTFTP error: trying to overwrite reserved memory...\n");
+ return;
+ }
printf("Using %s device\n", eth_get_name());
printf("Listening for TFTP transfer on %pI4\n", &net_ip);
- printf("Load address: 0x%lx\n", load_addr);
+ printf("Load address: 0x%lx\n", tftp_load_addr);
puts("Loading: *\b");
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread