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

[Michal Simek]

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 at 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 at amd.com> wrote:
>>>>
>>>> Hi Glass,
>>>>
>>>>> -----Original Message-----
>>>>> From: Simon Glass <sjg at chromium.org>
>>>>> Sent: Wednesday, August 7, 2024 3:21 AM
>>>>> To: Kummari, Prasad <Prasad.Kummari at amd.com>
>>>>> Cc: u-boot at lists.denx.de; git (AMD-Xilinx) <git at amd.com>; Simek, Michal
>>>>> <michal.simek at amd.com>; Abbarapu, Venkatesh
>>>>> <venkatesh.abbarapu at amd.com>; git at xilinx.com;
>>>>> jagan at amarulasolutions.com; n-francis at ti.com; d-gole at 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 at 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.

Thanks,
Michal