Re: [PATCH] cmd: sf: prevent overwriting the reserved memory

[Michal Simek <michal.simek@amd.com>]

Hi Simon,

On 8/9/24 17:58, Simon Glass wrote:
> Hi Michal,
> 
> On Fri, 9 Aug 2024 at 08:47, Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 8/9/24 16:44, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Thu, 8 Aug 2024 at 23:39, Michal Simek <michal.simek@amd.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 8/8/24 16:28, Simon Glass wrote:
>>>>> Hi Michal,
>>>>>
>>>>> On Wed, 7 Aug 2024 at 23:31, Michal Simek <michal.simek@amd.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/7/24 16:36, Simon Glass wrote:
>>>>>>> Hi Prasad,
>>>>>>>
>>>>>>> On Tue, 6 Aug 2024 at 23:05, Kummari, Prasad <Prasad.Kummari@amd.com> wrote:
>>>>>>>>
>>>>>>>> Hi Glass,
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>>> Sent: Wednesday, August 7, 2024 3:21 AM
>>>>>>>>> To: Kummari, Prasad <Prasad.Kummari@amd.com>
>>>>>>>>> Cc: u-boot@lists.denx.de; git (AMD-Xilinx) <git@amd.com>; Simek, Michal
>>>>>>>>> <michal.simek@amd.com>; Abbarapu, Venkatesh
>>>>>>>>> <venkatesh.abbarapu@amd.com>; git@xilinx.com;
>>>>>>>>> jagan@amarulasolutions.com; n-francis@ti.com; d-gole@ti.com
>>>>>>>>> Subject: Re: [PATCH] cmd: sf: prevent overwriting the reserved memory
>>>>>>>>>
>>>>>>>>> Caution: This message originated from an External Source. Use proper
>>>>>>>>> caution when opening attachments, clicking links, or responding.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Prasad,
>>>>>>>>>
>>>>>>>>> On Tue, 6 Aug 2024 at 06:08, Prasad Kummari <prasad.kummari@amd.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Added LMB API to prevent SF command from overwriting reserved memory
>>>>>>>>>> areas. The current SPI code does not use LMB APIs for loading data
>>>>>>>>>> into memory addresses. To resolve this, LMB APIs were added to check
>>>>>>>>>> the load address of an SF command and ensure it does not overwrite
>>>>>>>>>> reserved memory addresses. Similar checks are used in TFTP, serial
>>>>>>>>>> load, and boot code to prevent overwriting reserved memory.
>>>>>>>>>
>>>>>>>>> The SPI flash may be used to load other things, not just an OS. What is your
>>>>>>>>> use case or problem here?
>>>>>>>>
>>>>>>>> [Prasad]:  We have observed that SF command can overwrite the reserved area without throwing any errors or warnings.
>>>>>>>>      This issue was noticed when the TF-A area is reserved in the Device Tree at address 0xf000000. The sf command is
>>>>>>>>      corrupting the reserved area,  and U-Boot relocation address too.
>>>>>>>>
>>>>>>>> EX: TF-A reserved at ddr address 0xf000000
>>>>>>>>
>>>>>>>>           Versal NET> sf read 0x0f000000 0x0 0x100     ----> Overwriting reserved area.
>>>>>>>>           device 0 offset 0x0, size 0x100
>>>>>>>>           SF: 256 bytes @ 0x0 Read: OK
>>>>>>>>
>>>>>>>>          U-boot relocation address relocaddr   = 0x000000007fec2000
>>>>>>>>
>>>>>>>>           Versal NET> sf write 0x0000000077ec2000 0x0 0x100   --> Overwriting reserved area.
>>>>>>>>           device 0 offset 0x0, size 0x100
>>>>>>>>           SF: 256 bytes @ 0x0 Written: OK
>>>>>>>
>>>>>>> Yes. There are many things which can overwrite memory, e.g. the mw
>>>>>>> command. It is a boot loader so this is normal.
>>>>>>>
>>>>>>> What image are you loading here?
>>>>>>
>>>>>> In spi boot it can be Kernel/rootfs but at the end of day it doesn't really matter.
>>>>>
>>>>> OK, in that case yes it should use lmb. That was the question I was
>>>>> trying to understand.
>>>>>
>>>>>>
>>>>>> We have protection for srec, fs load, tftp and wget already.
>>>>>>
>>>>>> c6855195e4b4 ("loads: Block writes into LMB reserved areas of U-Boot")
>>>>>> aa3c609e2be5 ("fs: prevent overwriting reserved memory")
>>>>>> a156c47e39ad ("tftp: prevent overwriting reserved memory")
>>>>>> 04592adbdb99 ("net: wget: prevent overwriting reserved memory")
>>>>>>
>>>>>> And this is just +1 patch to protect sf command that it doesn't touch reserved
>>>>>> location.
>>>>>> The same code should be used for other commands(nand, usb, etc) which loading
>>>>>> block of data to memory because all of them shouldn't rewrite reserved memory.
>>>>>>
>>>>>> In connection to mw/mtest/etc command protection can be also done but not sure
>>>>>> if this is useful because you normally not using them for booting.
>>>>>
>>>>> Exactly.
>>>>>
>>>>> I am hoping that we can pull SPI flash into bootstd...has anyone
>>>>> looked at that? Are you using scripts or is there a special bootmeth?
>>>>
>>>> We didn't find this issue in connection to boot. As I wrote in another reply we
>>>> found it via spi testcases where TF-A was placed lower in DDR and test overwrite
>>>> it without any other evidence. Part of the reason is that protection units are
>>>> not enabled to protect secure FW.
>>>
>>> Do you mean the sandbox test test/dm/sf.c ? Or something else? If the
>>> former, then we could mark dm_test_spi_flash() with CONFIG_SANDBOX
>>
>> pytest one and I think it was this one.
>> https://github.com/Xilinx/u-boot-xlnx/blob/master/test/py/tests/test_spi.py
>>
>> Love is working on sending this test upstream as he did with others.
> 
> OK. But why is TF-A low in RAM? We really need to have a think about
> this TF-A thing. This is the second problem I've seen in a week (the
> first was rockchip resetting the timer). Is there a spec for what TF-A
> is supposed to do / not do?

It is user choice where they put trusted firmware.
All depends on their application. Normally TF-A is in OCM but some users can 
have a need to use OCM for user application because for example it is much 
faster than DDR.

Not sure if there is any official spec but documentation is here.
https://trustedfirmware-a.readthedocs.io/en/latest/

But this issue is not related to TF-A. It is just the way how we found it based 
on our partitioning.
Because DDR can be partitioned for Secure OS, different cpus (RPUs in our case) 
or for processing units(MB/Risc-V/other masters) running out of programmable 
logic. In general when you are reserved location for whatever reason all loading 
commands shouldn't use them.

Thanks,
Michal