[U-Boot] [PATCH 0/4] Fix CVE-2018-18440

[U-Boot] [PATCH 0/4] Fix CVE-2018-18440

[Simon Goldschmidt]

This series fixes CVE-2018-18440 ("insufficient boundary checks in
filesystem image load") by adding restrictions to the 'load'
command. 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.

Note that this doesn't yet fix CVE-2018-18439 ("insufficient
boundary checks in network image boot"), which is somewhat similar.

Note that patman warnings are in old code only or due to adopting
the file's coding style.

Simon Goldschmidt (4):
  lib: lmb: reserving overlapping regions should fail
  lib: lmb: add function lmb_alloc_addr
  fs: prevent overwriting reserved memory
  bootm: use new common function lmb_init_and_reserve

 common/bootm.c |  8 ++------
 fs/fs.c        | 56 +++++++++++++++++++++++++++++++++++++++++++++++---
 include/lmb.h  |  3 +++
 lib/lmb.c      | 42 +++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 9 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH 1/4] 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.

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

 lib/lmb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/lmb.c b/lib/lmb.c
index 1705417348..8dc703d996 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -136,6 +136,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;
 		}
 	}
 
-- 
2.17.1

[U-Boot] [PATCH 2/4] lib: lmb: add function lmb_alloc_addr

[Simon Goldschmidt]

This new function 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.

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

 include/lmb.h |  1 +
 lib/lmb.c     | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/lmb.h b/include/lmb.h
index f04d058093..bc06f175e1 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -38,6 +38,7 @@ 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 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 8dc703d996..c3af2fd5fc 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -324,6 +324,32 @@ 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))
+				return base;
+		}
+	}
+	return 0;
+}
+
 int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
 {
 	int i;
-- 
2.17.1

[U-Boot] [PATCH 3/4] 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>
---

 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 adae98d021..4baf6b1c39 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -428,13 +428,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.
@@ -451,6 +495,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)
 {
@@ -621,7 +671,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 bc06f175e1..810a37d5a5 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 c3af2fd5fc..99a4aaa09e 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -104,6 +104,19 @@ void lmb_init(struct lmb *lmb)
 	lmb->reserved.size = 0;
 }
 
+/* Initialize the struct, add memory and call arch/board reserve functions */
+void lmb_init_and_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size,
+			  void *fdt_blob)
+{
+	lmb_init(lmb);
+	lmb_add(lmb, base, size);
+	arch_lmb_reserve(lmb);
+	board_lmb_reserve(lmb);
+
+	if (IMAGE_ENABLE_OF_LIBFDT)
+		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
+}
+
 /* This routine called with relocation disabled. */
 static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t size)
 {
-- 
2.17.1

[U-Boot] [PATCH 4/4] 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>
---

 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 3/4] fs: prevent overwriting reserved memory

[Fabio Estevam]

Hi Simon,

On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:

> diff --git a/fs/fs.c b/fs/fs.c
> index adae98d021..4baf6b1c39 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -428,13 +428,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

Unrelated to your series, but I was wondering if we could get rid of
the CONFIG_LMB option.

As far as I can see all the architectures define it, the only
exception being arch/sh.

If you agree I can send a patch after your series gets applied that
removes CONFIG_LMB.

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Simon Goldschmidt]

On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
>
> > diff --git a/fs/fs.c b/fs/fs.c
> > index adae98d021..4baf6b1c39 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -428,13 +428,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
>
> Unrelated to your series, but I was wondering if we could get rid of
> the CONFIG_LMB option.
>
> As far as I can see all the architectures define it, the only
> exception being arch/sh.
>
> If you agree I can send a patch after your series gets applied that
> removes CONFIG_LMB.

Sure, that would clean things up.

Simon

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Heinrich Schuchardt]

On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> wrote:
>>
>> Hi Simon,
>>
>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>
>>> diff --git a/fs/fs.c b/fs/fs.c
>>> index adae98d021..4baf6b1c39 100644
>>> --- a/fs/fs.c
>>> +++ b/fs/fs.c
>>> @@ -428,13 +428,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
>>
>> Unrelated to your series, but I was wondering if we could get rid of
>> the CONFIG_LMB option.
>>
>> As far as I can see all the architectures define it, the only
>> exception being arch/sh.
>>
>> If you agree I can send a patch after your series gets applied that
>> removes CONFIG_LMB.
> 
> Sure, that would clean things up.
> 
> Simon
> 

NAK

This patch-series does not provide what is needed. With
odroid-c2_defconfig I get

fdt list /reserved-memory/secmon at 10000000
reserved-memory {
        secmon at 10000000 {
                reg = <0x00000000 0x10000000 0x00000000 0x00200000>;
                no-map;
        };
};

=> load mmc 0:1 0x10000000 dtb
22925 bytes read in 8 ms (2.7 MiB/s)

So now I have successfully overwritten the secure monitor. Urrgh.

As you have observed load is still writing into a memory area that is
reserved by the device-tree.

Please, iterate over the device tree to ensure that nothing is loaded
into a reserved memory area. Do not expect board files to do anything
but create the reserve-memory entry in the device tree.

Best regards

Heinrich

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Simon Goldschmidt]

On 13.11.2018 20:42, Heinrich Schuchardt wrote:
> On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>>
>>>> diff --git a/fs/fs.c b/fs/fs.c
>>>> index adae98d021..4baf6b1c39 100644
>>>> --- a/fs/fs.c
>>>> +++ b/fs/fs.c
>>>> @@ -428,13 +428,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
>>> Unrelated to your series, but I was wondering if we could get rid of
>>> the CONFIG_LMB option.
>>>
>>> As far as I can see all the architectures define it, the only
>>> exception being arch/sh.
>>>
>>> If you agree I can send a patch after your series gets applied that
>>> removes CONFIG_LMB.
>> Sure, that would clean things up.
>>
>> Simon
>>
> NAK
>
> This patch-series does not provide what is needed. With
> odroid-c2_defconfig I get
>
> fdt list /reserved-memory/secmon at 10000000
> reserved-memory {
>          secmon at 10000000 {
>                  reg = <0x00000000 0x10000000 0x00000000 0x00200000>;
>                  no-map;
>          };
> };
>
> => load mmc 0:1 0x10000000 dtb
> 22925 bytes read in 8 ms (2.7 MiB/s)
>
> So now I have successfully overwritten the secure monitor. Urrgh.
>
> As you have observed load is still writing into a memory area that is
> reserved by the device-tree.
>
> Please, iterate over the device tree to ensure that nothing is loaded
> into a reserved memory area. Do not expect board files to do anything
> but create the reserve-memory entry in the device tree.

First of all, thanks for testing. I must admit I haven't tested this 
case, I just had the impression the existing function 
'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll 
have to dig into why it doesn't.

Or do you have CONFIG_OF_LIBFDT disabled?

In any case, it *was* my intention to include the dts memory 
reservation! I'll make sure I test that for a v2.

Simon

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Heinrich Schuchardt]

On 11/13/18 9:01 PM, Simon Goldschmidt wrote:
> On 13.11.2018 20:42, Heinrich Schuchardt wrote:
>> On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
>>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com>
>>> wrote:
>>>> Hi Simon,
>>>>
>>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>>>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>>>
>>>>> diff --git a/fs/fs.c b/fs/fs.c
>>>>> index adae98d021..4baf6b1c39 100644
>>>>> --- a/fs/fs.c
>>>>> +++ b/fs/fs.c
>>>>> @@ -428,13 +428,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
>>>> Unrelated to your series, but I was wondering if we could get rid of
>>>> the CONFIG_LMB option.
>>>>
>>>> As far as I can see all the architectures define it, the only
>>>> exception being arch/sh.
>>>>
>>>> If you agree I can send a patch after your series gets applied that
>>>> removes CONFIG_LMB.
>>> Sure, that would clean things up.
>>>
>>> Simon
>>>
>> NAK
>>
>> This patch-series does not provide what is needed. With
>> odroid-c2_defconfig I get
>>
>> fdt list /reserved-memory/secmon at 10000000
>> reserved-memory {
>>          secmon at 10000000 {
>>                  reg = <0x00000000 0x10000000 0x00000000 0x00200000>;
>>                  no-map;
>>          };
>> };
>>
>> => load mmc 0:1 0x10000000 dtb
>> 22925 bytes read in 8 ms (2.7 MiB/s)
>>
>> So now I have successfully overwritten the secure monitor. Urrgh.
>>
>> As you have observed load is still writing into a memory area that is
>> reserved by the device-tree.
>>
>> Please, iterate over the device tree to ensure that nothing is loaded
>> into a reserved memory area. Do not expect board files to do anything
>> but create the reserve-memory entry in the device tree.
> 
> First of all, thanks for testing. I must admit I haven't tested this
> case, I just had the impression the existing function
> 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll
> have to dig into why it doesn't.
> 
> Or do you have CONFIG_OF_LIBFDT disabled?

`make odroid-c2_defconfig` sets
CONFIG_OF_LIBFDT=y

CONFIG_CMD_FDT depends on it. So without I would not have had the fdt
command used above.

The device-tree I was looking at was the one provided by U-Boot at
${fdtcontroladdr}.

We should also think about making this testable. I would be happy if we
had a test on a QEMU board.

Best regards

Heinrich

> 
> In any case, it *was* my intention to include the dts memory
> reservation! I'll make sure I test that for a v2.
> 
> Simon
> 

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Simon Goldschmidt]

Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de>
geschrieben:

> On 11/13/18 9:01 PM, Simon Goldschmidt wrote:
> > On 13.11.2018 20:42, Heinrich Schuchardt wrote:
> >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
> >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com>
> >>> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
> >>>> <simon.k.r.goldschmidt@gmail.com> wrote:
> >>>>
> >>>>> diff --git a/fs/fs.c b/fs/fs.c
> >>>>> index adae98d021..4baf6b1c39 100644
> >>>>> --- a/fs/fs.c
> >>>>> +++ b/fs/fs.c
> >>>>> @@ -428,13 +428,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
> >>>> Unrelated to your series, but I was wondering if we could get rid of
> >>>> the CONFIG_LMB option.
> >>>>
> >>>> As far as I can see all the architectures define it, the only
> >>>> exception being arch/sh.
> >>>>
> >>>> If you agree I can send a patch after your series gets applied that
> >>>> removes CONFIG_LMB.
> >>> Sure, that would clean things up.
> >>>
> >>> Simon
> >>>
> >> NAK
> >>
> >> This patch-series does not provide what is needed. With
> >> odroid-c2_defconfig I get
> >>
> >> fdt list /reserved-memory/secmon at 10000000
> >> reserved-memory {
> >>          secmon at 10000000 {
> >>                  reg = <0x00000000 0x10000000 0x00000000 0x00200000>;
> >>                  no-map;
> >>          };
> >> };
> >>
> >> => load mmc 0:1 0x10000000 dtb
> >> 22925 bytes read in 8 ms (2.7 MiB/s)
> >>
> >> So now I have successfully overwritten the secure monitor. Urrgh.
> >>
> >> As you have observed load is still writing into a memory area that is
> >> reserved by the device-tree.
> >>
> >> Please, iterate over the device tree to ensure that nothing is loaded
> >> into a reserved memory area. Do not expect board files to do anything
> >> but create the reserve-memory entry in the device tree.
> >
> > First of all, thanks for testing. I must admit I haven't tested this
> > case, I just had the impression the existing function
> > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll
> > have to dig into why it doesn't.
> >
> > Or do you have CONFIG_OF_LIBFDT disabled?
>
> `make odroid-c2_defconfig` sets
> CONFIG_OF_LIBFDT=y
>
> CONFIG_CMD_FDT depends on it. So without I would not have had the fdt
> command used above.
>
> The device-tree I was looking at was the one provided by U-Boot at
> ${fdtcontroladdr}.
>

That might be an explanation. I used 'gd->fdt_blob' only in my patch. Are
there any more device tree locations to care about?

We should also think about making this testable. I would be happy if we
> had a test on a QEMU board.
>

I tried to build the tests but I only got build errors. Is there any
documentation about what I could be missing? I think my Ubuntu should be up
to date, so maybe I am missing some dependencies...?

Simon


> Best regards
>
> Heinrich
>
> >
> > In any case, it *was* my intention to include the dts memory
> > reservation! I'll make sure I test that for a v2.
> >
> > Simon
> >
>
>

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Heinrich Schuchardt]

On 11/13/18 10:47 PM, Simon Goldschmidt wrote:
> 
> 
> Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> geschrieben:
> 
>     On 11/13/18 9:01 PM, Simon Goldschmidt wrote:
>     > On 13.11.2018 20:42, Heinrich Schuchardt wrote:
>     >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
>     >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam
>     <festevam at gmail.com <mailto:festevam@gmail.com>>
>     >>> wrote:
>     >>>> Hi Simon,
>     >>>>
>     >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>     >>>> <simon.k.r.goldschmidt@gmail.com
>     <mailto:simon.k.r.goldschmidt@gmail.com>> wrote:
>     >>>>
>     >>>>> diff --git a/fs/fs.c b/fs/fs.c
>     >>>>> index adae98d021..4baf6b1c39 100644
>     >>>>> --- a/fs/fs.c
>     >>>>> +++ b/fs/fs.c
>     >>>>> @@ -428,13 +428,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
>     >>>> Unrelated to your series, but I was wondering if we could get
>     rid of
>     >>>> the CONFIG_LMB option.
>     >>>>
>     >>>> As far as I can see all the architectures define it, the only
>     >>>> exception being arch/sh.
>     >>>>
>     >>>> If you agree I can send a patch after your series gets applied that
>     >>>> removes CONFIG_LMB.
>     >>> Sure, that would clean things up.
>     >>>
>     >>> Simon
>     >>>
>     >> NAK
>     >>
>     >> This patch-series does not provide what is needed. With
>     >> odroid-c2_defconfig I get
>     >>
>     >> fdt list /reserved-memory/secmon at 10000000
>     >> reserved-memory {
>     >>          secmon at 10000000 {
>     >>                  reg = <0x00000000 0x10000000 0x00000000 0x00200000>;
>     >>                  no-map;
>     >>          };
>     >> };
>     >>
>     >> => load mmc 0:1 0x10000000 dtb
>     >> 22925 bytes read in 8 ms (2.7 MiB/s)
>     >>
>     >> So now I have successfully overwritten the secure monitor. Urrgh.
>     >>
>     >> As you have observed load is still writing into a memory area that is
>     >> reserved by the device-tree.
>     >>
>     >> Please, iterate over the device tree to ensure that nothing is loaded
>     >> into a reserved memory area. Do not expect board files to do anything
>     >> but create the reserve-memory entry in the device tree.
>     >
>     > First of all, thanks for testing. I must admit I haven't tested this
>     > case, I just had the impression the existing function
>     > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll
>     > have to dig into why it doesn't.
>     >
>     > Or do you have CONFIG_OF_LIBFDT disabled?
> 
>     `make odroid-c2_defconfig` sets
>     CONFIG_OF_LIBFDT=y
> 
>     CONFIG_CMD_FDT depends on it. So without I would not have had the fdt
>     command used above.
> 
>     The device-tree I was looking at was the one provided by U-Boot at
>     ${fdtcontroladdr}.
> 
> 
> That might be an explanation. I used 'gd->fdt_blob' only in my patch.

For the Odroid C2 the relevant memory reservations are created in
arch/arm/mach-meson/board.c:61:
void meson_gx_init_reserved_memory(void *fdt)

According to /README ${fdtcontroladdr} and gd->fdt_blob should be the same:

lib/fdtdec.c:1255:
gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16,
(uintptr_t)gd->fdt_blob);

The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board
file (board/broadcom/bcmstb/bcmstb.c).

You should use gd->fdt_blob. Just make sure it is already set.

Best regards

Heinrich

> Are there any more device tree locations to care about?
> 
>     We should also think about making this testable. I would be happy if we
>     had a test on a QEMU board.
> 
> 
> I tried to build the tests but I only got build errors. Is there any
> documentation about what I could be missing? I think my Ubuntu should be
> up to date, so maybe I am missing some dependencies...?
> 
> Simon
> 
> 
>     Best regards
> 
>     Heinrich
> 
>     >
>     > In any case, it *was* my intention to include the dts memory
>     > reservation! I'll make sure I test that for a v2.
>     >
>     > Simon
>     >
> 

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Simon Goldschmidt]

On 14.11.2018 00:03, Heinrich Schuchardt wrote:
> On 11/13/18 10:47 PM, Simon Goldschmidt wrote:
>>
>> Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de
>> <mailto:xypron.glpk@gmx.de>> geschrieben:
>>
>>      On 11/13/18 9:01 PM, Simon Goldschmidt wrote:
>>      > On 13.11.2018 20:42, Heinrich Schuchardt wrote:
>>      >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
>>      >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam
>>      <festevam at gmail.com <mailto:festevam@gmail.com>>
>>      >>> wrote:
>>      >>>> Hi Simon,
>>      >>>>
>>      >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>>      >>>> <simon.k.r.goldschmidt@gmail.com
>>      <mailto:simon.k.r.goldschmidt@gmail.com>> wrote:
>>      >>>>
>>      >>>>> diff --git a/fs/fs.c b/fs/fs.c
>>      >>>>> index adae98d021..4baf6b1c39 100644
>>      >>>>> --- a/fs/fs.c
>>      >>>>> +++ b/fs/fs.c
>>      >>>>> @@ -428,13 +428,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
>>      >>>> Unrelated to your series, but I was wondering if we could get
>>      rid of
>>      >>>> the CONFIG_LMB option.
>>      >>>>
>>      >>>> As far as I can see all the architectures define it, the only
>>      >>>> exception being arch/sh.
>>      >>>>
>>      >>>> If you agree I can send a patch after your series gets applied that
>>      >>>> removes CONFIG_LMB.
>>      >>> Sure, that would clean things up.
>>      >>>
>>      >>> Simon
>>      >>>
>>      >> NAK
>>      >>
>>      >> This patch-series does not provide what is needed. With
>>      >> odroid-c2_defconfig I get
>>      >>
>>      >> fdt list /reserved-memory/secmon at 10000000
>>      >> reserved-memory {
>>      >>          secmon at 10000000 {
>>      >>                  reg = <0x00000000 0x10000000 0x00000000 0x00200000>;
>>      >>                  no-map;
>>      >>          };
>>      >> };
>>      >>
>>      >> => load mmc 0:1 0x10000000 dtb
>>      >> 22925 bytes read in 8 ms (2.7 MiB/s)
>>      >>
>>      >> So now I have successfully overwritten the secure monitor. Urrgh.
>>      >>
>>      >> As you have observed load is still writing into a memory area that is
>>      >> reserved by the device-tree.
>>      >>
>>      >> Please, iterate over the device tree to ensure that nothing is loaded
>>      >> into a reserved memory area. Do not expect board files to do anything
>>      >> but create the reserve-memory entry in the device tree.
>>      >
>>      > First of all, thanks for testing. I must admit I haven't tested this
>>      > case, I just had the impression the existing function
>>      > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll
>>      > have to dig into why it doesn't.
>>      >
>>      > Or do you have CONFIG_OF_LIBFDT disabled?
>>
>>      `make odroid-c2_defconfig` sets
>>      CONFIG_OF_LIBFDT=y
>>
>>      CONFIG_CMD_FDT depends on it. So without I would not have had the fdt
>>      command used above.
>>
>>      The device-tree I was looking at was the one provided by U-Boot at
>>      ${fdtcontroladdr}.
>>
>>
>> That might be an explanation. I used 'gd->fdt_blob' only in my patch.
> For the Odroid C2 the relevant memory reservations are created in
> arch/arm/mach-meson/board.c:61:
> void meson_gx_init_reserved_memory(void *fdt)
>
> According to /README ${fdtcontroladdr} and gd->fdt_blob should be the same:
>
> lib/fdtdec.c:1255:
> gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16,
> (uintptr_t)gd->fdt_blob);
>
> The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board
> file (board/broadcom/bcmstb/bcmstb.c).
>
> You should use gd->fdt_blob. Just make sure it is already set.

OK, so it seems U-Boot just cannot handle the "reserved-memory" node 
with its subnodes but only the "memreserve" thing on top level?
I have the rest of the patches in a state I would submit, but I'll need 
some more time to dig into the dts reserved memory reservation...

Simon

>
> Best regards
>
> Heinrich
>
>> Are there any more device tree locations to care about?
>>
>>      We should also think about making this testable. I would be happy if we
>>      had a test on a QEMU board.
>>
>>
>> I tried to build the tests but I only got build errors. Is there any
>> documentation about what I could be missing? I think my Ubuntu should be
>> up to date, so maybe I am missing some dependencies...?
>>
>> Simon
>>
>>
>>      Best regards
>>
>>      Heinrich
>>
>>      >
>>      > In any case, it *was* my intention to include the dts memory
>>      > reservation! I'll make sure I test that for a v2.
>>      >
>>      > Simon
>>      >
>>

[U-Boot] [PATCH 3/4] fs: prevent overwriting reserved memory

[Heinrich Schuchardt]

On 11/16/18 7:48 AM, Simon Goldschmidt wrote:
> On 14.11.2018 00:03, Heinrich Schuchardt wrote:
>> On 11/13/18 10:47 PM, Simon Goldschmidt wrote:
>>>
>>> Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de
>>> <mailto:xypron.glpk@gmx.de>> geschrieben:
>>>
>>>      On 11/13/18 9:01 PM, Simon Goldschmidt wrote:
>>>      > On 13.11.2018 20:42, Heinrich Schuchardt wrote:
>>>      >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote:
>>>      >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam
>>>      <festevam at gmail.com <mailto:festevam@gmail.com>>
>>>      >>> wrote:
>>>      >>>> Hi Simon,
>>>      >>>>
>>>      >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>>>      >>>> <simon.k.r.goldschmidt@gmail.com
>>>      <mailto:simon.k.r.goldschmidt@gmail.com>> wrote:
>>>      >>>>
>>>      >>>>> diff --git a/fs/fs.c b/fs/fs.c
>>>      >>>>> index adae98d021..4baf6b1c39 100644
>>>      >>>>> --- a/fs/fs.c
>>>      >>>>> +++ b/fs/fs.c
>>>      >>>>> @@ -428,13 +428,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
>>>      >>>> Unrelated to your series, but I was wondering if we could get
>>>      rid of
>>>      >>>> the CONFIG_LMB option.
>>>      >>>>
>>>      >>>> As far as I can see all the architectures define it, the only
>>>      >>>> exception being arch/sh.
>>>      >>>>
>>>      >>>> If you agree I can send a patch after your series gets
>>> applied that
>>>      >>>> removes CONFIG_LMB.
>>>      >>> Sure, that would clean things up.
>>>      >>>
>>>      >>> Simon
>>>      >>>
>>>      >> NAK
>>>      >>
>>>      >> This patch-series does not provide what is needed. With
>>>      >> odroid-c2_defconfig I get
>>>      >>
>>>      >> fdt list /reserved-memory/secmon at 10000000
>>>      >> reserved-memory {
>>>      >>          secmon at 10000000 {
>>>      >>                  reg = <0x00000000 0x10000000 0x00000000
>>> 0x00200000>;
>>>      >>                  no-map;
>>>      >>          };
>>>      >> };
>>>      >>
>>>      >> => load mmc 0:1 0x10000000 dtb
>>>      >> 22925 bytes read in 8 ms (2.7 MiB/s)
>>>      >>
>>>      >> So now I have successfully overwritten the secure monitor.
>>> Urrgh.
>>>      >>
>>>      >> As you have observed load is still writing into a memory area
>>> that is
>>>      >> reserved by the device-tree.
>>>      >>
>>>      >> Please, iterate over the device tree to ensure that nothing
>>> is loaded
>>>      >> into a reserved memory area. Do not expect board files to do
>>> anything
>>>      >> but create the reserve-memory entry in the device tree.
>>>      >
>>>      > First of all, thanks for testing. I must admit I haven't
>>> tested this
>>>      > case, I just had the impression the existing function
>>>      > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do
>>> that. I'll
>>>      > have to dig into why it doesn't.
>>>      >
>>>      > Or do you have CONFIG_OF_LIBFDT disabled?
>>>
>>>      `make odroid-c2_defconfig` sets
>>>      CONFIG_OF_LIBFDT=y
>>>
>>>      CONFIG_CMD_FDT depends on it. So without I would not have had
>>> the fdt
>>>      command used above.
>>>
>>>      The device-tree I was looking at was the one provided by U-Boot at
>>>      ${fdtcontroladdr}.
>>>
>>>
>>> That might be an explanation. I used 'gd->fdt_blob' only in my patch.
>> For the Odroid C2 the relevant memory reservations are created in
>> arch/arm/mach-meson/board.c:61:
>> void meson_gx_init_reserved_memory(void *fdt)
>>
>> According to /README ${fdtcontroladdr} and gd->fdt_blob should be the
>> same:
>>
>> lib/fdtdec.c:1255:
>> gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16,
>> (uintptr_t)gd->fdt_blob);
>>
>> The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board
>> file (board/broadcom/bcmstb/bcmstb.c).
>>
>> You should use gd->fdt_blob. Just make sure it is already set.
> 
> OK, so it seems U-Boot just cannot handle the "reserved-memory" node
> with its subnodes but only the "memreserve" thing on top level?
> I have the rest of the patches in a state I would submit, but I'll need
> some more time to dig into the dts reserved memory reservation...
> 
> Simon
> 

Linux
Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
defines /reserved-memory.

Documentation/arm64/booting.txt defines /memreserve

The device tree specification v0.2 fails to provide the label used for
memory reservations.

https://patchwork.kernel.org/patch/5143721/ sheds some light:

"Device tree regions described by /memreserve/ entries are not available
in /proc/device-tree, and hence are not available to user space
utilities that use /proc/device-tree.  In order for these regions to be
available, convert any arm64 DTS files using /memreserve/ entries to use
reserved-memory nodes."

So /memreserve and /reserved-memory have different semantics but neither
region should be used by the load command.

Best regards

Heinrich

>>
>> Best regards
>>
>> Heinrich
>>
>>> Are there any more device tree locations to care about?
>>>
>>>      We should also think about making this testable. I would be
>>> happy if we
>>>      had a test on a QEMU board.
>>>
>>>
>>> I tried to build the tests but I only got build errors. Is there any
>>> documentation about what I could be missing? I think my Ubuntu should be
>>> up to date, so maybe I am missing some dependencies...?
>>>
>>> Simon
>>>
>>>
>>>      Best regards
>>>
>>>      Heinrich
>>>
>>>      >
>>>      > In any case, it *was* my intention to include the dts memory
>>>      > reservation! I'll make sure I test that for a v2.
>>>      >
>>>      > Simon
>>>      >
>>>
> 
>