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

[Heinrich Schuchardt]

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

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
>