Hi Michal,
On Mon, 26 Aug 2024 at 02:48, Michal Simek <michal.si...@amd.com> wrote:
>
> Hi Simon,
>
> On 8/9/24 17:58, Simon Glass wrote:
> > Hi Michal,
> >
> > On Fri, 9 Aug 2024 at 08:47, Michal Simek <michal.si...@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.si...@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.si...@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.kumm...@amd.com>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> Hi Glass,
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Simon Glass <s...@chromium.org>
> >>>>>>>>> Sent: Wednesday, August 7, 2024 3:21 AM
> >>>>>>>>> To: Kummari, Prasad <prasad.kumm...@amd.com>
> >>>>>>>>> Cc: u-boot@lists.denx.de; git (AMD-Xilinx) <g...@amd.com>; Simek,
> >>>>>>>>> Michal
> >>>>>>>>> <michal.si...@amd.com>; Abbarapu, Venkatesh
> >>>>>>>>> <venkatesh.abbar...@amd.com>; g...@xilinx.com;
> >>>>>>>>> ja...@amarulasolutions.com; n-fran...@ti.com; d-g...@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.kumm...@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.
OK thanks for the info.
Regards,
Simon
