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

[Simon Goldschmidt]

Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <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>
> >>> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt
> >>>> <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. 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
> >
>
>