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

Fri Nov 16 18:47:08 UTC 2018

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

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