[U-Boot] [PATCH 0/4] Fix CVE-2018-18440
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(-)
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; } }
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;
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) {
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.
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
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@10000000 reserved-memory { secmon@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
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@10000000 reserved-memory { secmon@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
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@10000000 reserved-memory { secmon@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
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@10000000 reserved-memory { secmon@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
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@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@10000000 >> reserved-memory { >> secmon@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 >
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@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@10000000 >> reserved-memory { >> secmon@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 >
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@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@10000000 >> reserved-memory { >> secmon@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 >
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)
participants (3)
-
Fabio Estevam -
Heinrich Schuchardt -
Simon Goldschmidt