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

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