[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 at gmx.de
> <mailto:xypron.glpk at 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 at gmail.com>>
>     >>> wrote:
>     >>>> Hi Simon,
>     >>>>
>     >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
>     >>>> <simon.k.r.goldschmidt at gmail.com
>     <mailto:simon.k.r.goldschmidt at 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
>     >
>