diff mbox series

cmd: sf: prevent overwriting the reserved memory

Message ID 20240806120659.686073-1-prasad.kummari@amd.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series cmd: sf: prevent overwriting the reserved memory | expand

Commit Message

Prasad Kummari Aug. 6, 2024, 12:07 p.m. UTC
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.

Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
---
UT:
Tested on Versal NET board

Versal NET> sf read 0xf000000 0x0 0x100                                         
device 0 offset 0x0, size 0x100                                                 
ERROR: trying to overwrite reserved memory...
                                   
Versal NET> sf read 0x79ea9000 0x 0x100                                         
device 0 offset 0x0, size 0x100                                                 
ERROR: trying to overwrite reserved memory...       
                            
Versal NET> sf write 0x79ea9000 0x 0x100                                        
device 0 offset 0x0, size 0x100                                                 
ERROR: trying to overwrite reserved memory...

Versal NET> sf write 0x79eaf000 0x20000 0x100 
device 0 offset 0x20000, size 0x100
ERROR: trying to overwrite reserved memory...

 cmd/sf.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Comments

Simon Glass Aug. 6, 2024, 9:50 p.m. UTC | #1
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?

Regards,
Simon


>
> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
> ---
> UT:
> Tested on Versal NET board
>
> Versal NET> sf read 0xf000000 0x0 0x100
> device 0 offset 0x0, size 0x100
> ERROR: trying to overwrite reserved memory...
>
> Versal NET> sf read 0x79ea9000 0x 0x100
> device 0 offset 0x0, size 0x100
> ERROR: trying to overwrite reserved memory...
>
> Versal NET> sf write 0x79ea9000 0x 0x100
> device 0 offset 0x0, size 0x100
> ERROR: trying to overwrite reserved memory...
>
> Versal NET> sf write 0x79eaf000 0x20000 0x100
> device 0 offset 0x20000, size 0x100
> ERROR: trying to overwrite reserved memory...
>
>  cmd/sf.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/sf.c b/cmd/sf.c
> index f43a2e08b3..d2ce1af8a0 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -10,6 +10,7 @@
>  #include <div64.h>
>  #include <dm.h>
>  #include <log.h>
> +#include <lmb.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <spi.h>
> @@ -272,6 +273,35 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
>         return 0;
>  }
>
> +#ifdef CONFIG_LMB
> +static int do_spi_read_lmb_check(ulong start_addr, loff_t len)
> +{
> +       struct lmb lmb;
> +       phys_size_t max_size;
> +       ulong end_addr;
> +
> +       lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
> +       lmb_dump_all(&lmb);
> +
> +       max_size = lmb_get_free_size(&lmb, start_addr);
> +       if (!max_size) {
> +               printf("ERROR: trying to overwrite reserved memory...\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       end_addr = start_addr + max_size;
> +       if (!end_addr)
> +               end_addr = ULONG_MAX;
> +
> +       if ((start_addr + len) > end_addr) {
> +               printf("ERROR: trying to overwrite reserved memory...\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static int do_spi_flash_read_write(int argc, char *const argv[])
>  {
>         unsigned long addr;
> @@ -315,7 +345,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[])
>                 ret = spi_flash_update(flash, offset, len, buf);
>         } else if (strncmp(argv[0], "read", 4) == 0 ||
>                         strncmp(argv[0], "write", 5) == 0) {
> -               int read;
> +               int read, ret;
> +
> +#ifdef CONFIG_LMB
> +               ret = do_spi_read_lmb_check(addr, len);
> +               if (ret)
> +                       return ret;
> +#endif
>
>                 read = strncmp(argv[0], "read", 4) == 0;
>                 if (read)
> --
> 2.25.1
>
Prasad Kummari Aug. 7, 2024, 5:05 a.m. UTC | #2
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

> 
> Regards,
> Simon
> 
> 
> >
> > Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
> > ---
> > UT:
> > Tested on Versal NET board
> >
> > Versal NET> sf read 0xf000000 0x0 0x100 device 0 offset 0x0, size
> > 0x100
> > ERROR: trying to overwrite reserved memory...
> >
> > Versal NET> sf read 0x79ea9000 0x 0x100 device 0 offset 0x0, size
> > 0x100
> > ERROR: trying to overwrite reserved memory...
> >
> > Versal NET> sf write 0x79ea9000 0x 0x100 device 0 offset 0x0, size
> > 0x100
> > ERROR: trying to overwrite reserved memory...
> >
> > Versal NET> sf write 0x79eaf000 0x20000 0x100 device 0 offset 0x20000,
> > size 0x100
> > ERROR: trying to overwrite reserved memory...
> >
> >  cmd/sf.c | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/cmd/sf.c b/cmd/sf.c
> > index f43a2e08b3..d2ce1af8a0 100644
> > --- a/cmd/sf.c
> > +++ b/cmd/sf.c
> > @@ -10,6 +10,7 @@
> >  #include <div64.h>
> >  #include <dm.h>
> >  #include <log.h>
> > +#include <lmb.h>
> >  #include <malloc.h>
> >  #include <mapmem.h>
> >  #include <spi.h>
> > @@ -272,6 +273,35 @@ static int spi_flash_update(struct spi_flash *flash,
> u32 offset,
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_LMB
> > +static int do_spi_read_lmb_check(ulong start_addr, loff_t len) {
> > +       struct lmb lmb;
> > +       phys_size_t max_size;
> > +       ulong end_addr;
> > +
> > +       lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
> > +       lmb_dump_all(&lmb);
> > +
> > +       max_size = lmb_get_free_size(&lmb, start_addr);
> > +       if (!max_size) {
> > +               printf("ERROR: trying to overwrite reserved memory...\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       end_addr = start_addr + max_size;
> > +       if (!end_addr)
> > +               end_addr = ULONG_MAX;
> > +
> > +       if ((start_addr + len) > end_addr) {
> > +               printf("ERROR: trying to overwrite reserved memory...\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> >  static int do_spi_flash_read_write(int argc, char *const argv[])  {
> >         unsigned long addr;
> > @@ -315,7 +345,13 @@ static int do_spi_flash_read_write(int argc, char
> *const argv[])
> >                 ret = spi_flash_update(flash, offset, len, buf);
> >         } else if (strncmp(argv[0], "read", 4) == 0 ||
> >                         strncmp(argv[0], "write", 5) == 0) {
> > -               int read;
> > +               int read, ret;
> > +
> > +#ifdef CONFIG_LMB
> > +               ret = do_spi_read_lmb_check(addr, len);
> > +               if (ret)
> > +                       return ret;
> > +#endif
> >
> >                 read = strncmp(argv[0], "read", 4) == 0;
> >                 if (read)
> > --
> > 2.25.1
> >
Simon Glass Aug. 7, 2024, 2:36 p.m. UTC | #3
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?

Regards,
Simon
Prasad Kummari Aug. 7, 2024, 2:46 p.m. UTC | #4
Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Wednesday, August 7, 2024 8:06 PM
> 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 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?

[Prasad] : We are loading TF-A(bl31.elf) at ddr address 0xf000000.

> 
> Regards,
> Simon
Simon Glass Aug. 7, 2024, 8:53 p.m. UTC | #5
Hi Prasad,

On Wed, 7 Aug 2024 at 08:46, Kummari, Prasad <Prasad.Kummari@amd.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Glass <sjg@chromium.org>
> > Sent: Wednesday, August 7, 2024 8:06 PM
> > 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 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?
>
> [Prasad] : We are loading TF-A(bl31.elf) at ddr address 0xf000000.

I mean what image are you loading which overwrites TF-A?

Regards,
Simon
Tom Rini Aug. 7, 2024, 9:12 p.m. UTC | #6
On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari 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.
> 
> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>

This is a much more generic issue that should be looked in to with the
LMB rewrite that Sughosh is working on.
Michal Simek Aug. 8, 2024, 5:31 a.m. UTC | #7
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.

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.

Thanks,
Michal
Michal Simek Aug. 8, 2024, 5:35 a.m. UTC | #8
On 8/7/24 23:12, Tom Rini wrote:
> On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari 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.
>>
>> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
> 
> This is a much more generic issue that should be looked in to with the
> LMB rewrite that Sughosh is working on.

yes. And is it going to be the part of his series?
I expect that if he accepts this will be done on the top of it and there is 
likely no reason to wait.
We find the issue out on system which has more dynamic behavior that spi pytest 
read the whole memory and rewrite TF-A.

And currently we are in situation where some commands are checking reserved 
location and some of them not.

Thanks,
Michal
Sughosh Ganu Aug. 8, 2024, 6:22 a.m. UTC | #9
On Thu, 8 Aug 2024 at 11:05, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 8/7/24 23:12, Tom Rini wrote:
> > On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari 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.
> >>
> >> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
> >
> > This is a much more generic issue that should be looked in to with the
> > LMB rewrite that Sughosh is working on.
>
> yes. And is it going to be the part of his series?
> I expect that if he accepts this will be done on the top of it and there is
> likely no reason to wait.

This change would be needed, but in a different form. The patch, since
based on the current master branch, is assuming a local lmb memory
map. My series is doing away with that, and so we will no longer have
the lmb_init_and_reserve() API, for example. I would suggest that the
patch be put out either on top of my patches, or ideally, once the lmb
patches get merged.


> We find the issue out on system which has more dynamic behavior that spi pytest
> read the whole memory and rewrite TF-A.
>
> And currently we are in situation where some commands are checking reserved
> location and some of them not.

Like you mention in another email, there are instances where we have
commands which are loading images to memory, but are not considering
the lmb reservations. These should be fixed, similar to what this
patch is attempting. Maybe we need a common function where the load
address gets checked with the lmb module, similar to
fs_read_lmb_check(), and then have this called from the various
commands?

-sughosh

>
> Thanks,
> Michal
Michal Simek Aug. 8, 2024, 11:18 a.m. UTC | #10
On 8/8/24 08:22, Sughosh Ganu wrote:
> On Thu, 8 Aug 2024 at 11:05, Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 8/7/24 23:12, Tom Rini wrote:
>>> On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari 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.
>>>>
>>>> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
>>>
>>> This is a much more generic issue that should be looked in to with the
>>> LMB rewrite that Sughosh is working on.
>>
>> yes. And is it going to be the part of his series?
>> I expect that if he accepts this will be done on the top of it and there is
>> likely no reason to wait.
> 
> This change would be needed, but in a different form. The patch, since
> based on the current master branch, is assuming a local lmb memory
> map. My series is doing away with that, and so we will no longer have
> the lmb_init_and_reserve() API, for example. I would suggest that the
> patch be put out either on top of my patches, or ideally, once the lmb
> patches get merged.

I care that we can't overwrite reserved memory by any of load commands.
Better to be fixed earlier rather than later but up to Tom to decide.
 From my perspective this is incorrect behavior which is fixing issue and likely 
this can go to 2024.10 version.
Your LMB series is likely going to target 2025.01.

>> We find the issue out on system which has more dynamic behavior that spi pytest
>> read the whole memory and rewrite TF-A.
>>
>> And currently we are in situation where some commands are checking reserved
>> location and some of them not.
> 
> Like you mention in another email, there are instances where we have
> commands which are loading images to memory, but are not considering
> the lmb reservations. These should be fixed, similar to what this
> patch is attempting. Maybe we need a common function where the load
> address gets checked with the lmb module, similar to
> fs_read_lmb_check(), and then have this called from the various
> commands?

yes, it make sense not to have 4 different functions in different drivers but 
call only one. Definitely nice to have it on the top of your LMB series to be 
able to use it in nand cmd for example.

Thanks,
Michal
Simon Glass Aug. 8, 2024, 2:28 p.m. UTC | #11
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?

Regards,
Simon
Tom Rini Aug. 8, 2024, 3:46 p.m. UTC | #12
On Thu, Aug 08, 2024 at 01:18:44PM +0200, Michal Simek wrote:
> 
> 
> On 8/8/24 08:22, Sughosh Ganu wrote:
> > On Thu, 8 Aug 2024 at 11:05, Michal Simek <michal.simek@amd.com> wrote:
> > > 
> > > 
> > > 
> > > On 8/7/24 23:12, Tom Rini wrote:
> > > > On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari 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.
> > > > > 
> > > > > Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
> > > > 
> > > > This is a much more generic issue that should be looked in to with the
> > > > LMB rewrite that Sughosh is working on.
> > > 
> > > yes. And is it going to be the part of his series?
> > > I expect that if he accepts this will be done on the top of it and there is
> > > likely no reason to wait.
> > 
> > This change would be needed, but in a different form. The patch, since
> > based on the current master branch, is assuming a local lmb memory
> > map. My series is doing away with that, and so we will no longer have
> > the lmb_init_and_reserve() API, for example. I would suggest that the
> > patch be put out either on top of my patches, or ideally, once the lmb
> > patches get merged.
> 
> I care that we can't overwrite reserved memory by any of load commands.
> Better to be fixed earlier rather than later but up to Tom to decide.
> From my perspective this is incorrect behavior which is fixing issue and
> likely this can go to 2024.10 version.
> Your LMB series is likely going to target 2025.01.

So, from my point of view, this is a longstanding issue that I get why
people are concerned, but I think it's missing a bigger point. For
network loads, OK, no one needs physical access to do something
malicious, so yes, it's important we check there every time. For
filesystem loads? There's far far too many production devices using SD
cards, so yes, a malicious actor needs physical access, but not much.
For flash (SPI or NAND), at that point why doesn't the malicious actor
just use "mw" instead? The device is in their position if they're able
to hook up probes/etc.

So my current thought process is that yes, fixing SPI and NAND and all
of the other forms of reading (outside of "cp") need to be fixed, as a
follow-up series to what Sughosh is doing. And then reminding people
that CMD_MEMORY is dangerous and perhaps think about splitting mw/etc
out from "cmp/base/loop" so that CMD_MEMORY can be disabled for boards
that want a more secure feel.

I'm open to being convinced I'm wrong and this is a serious problem to
address now, not later, however.
Michal Simek Aug. 9, 2024, 5:39 a.m. UTC | #13
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.

Thanks,
Michal
Michal Simek Aug. 9, 2024, 5:41 a.m. UTC | #14
Hi Tom,

On 8/8/24 17:46, Tom Rini wrote:
> On Thu, Aug 08, 2024 at 01:18:44PM +0200, Michal Simek wrote:
>>
>>
>> On 8/8/24 08:22, Sughosh Ganu wrote:
>>> On Thu, 8 Aug 2024 at 11:05, Michal Simek <michal.simek@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 8/7/24 23:12, Tom Rini wrote:
>>>>> On Tue, Aug 06, 2024 at 05:37:00PM +0530, Prasad Kummari 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.
>>>>>>
>>>>>> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com>
>>>>>
>>>>> This is a much more generic issue that should be looked in to with the
>>>>> LMB rewrite that Sughosh is working on.
>>>>
>>>> yes. And is it going to be the part of his series?
>>>> I expect that if he accepts this will be done on the top of it and there is
>>>> likely no reason to wait.
>>>
>>> This change would be needed, but in a different form. The patch, since
>>> based on the current master branch, is assuming a local lmb memory
>>> map. My series is doing away with that, and so we will no longer have
>>> the lmb_init_and_reserve() API, for example. I would suggest that the
>>> patch be put out either on top of my patches, or ideally, once the lmb
>>> patches get merged.
>>
>> I care that we can't overwrite reserved memory by any of load commands.
>> Better to be fixed earlier rather than later but up to Tom to decide.
>>  From my perspective this is incorrect behavior which is fixing issue and
>> likely this can go to 2024.10 version.
>> Your LMB series is likely going to target 2025.01.
> 
> So, from my point of view, this is a longstanding issue that I get why
> people are concerned, but I think it's missing a bigger point. For
> network loads, OK, no one needs physical access to do something
> malicious, so yes, it's important we check there every time. For
> filesystem loads? There's far far too many production devices using SD
> cards, so yes, a malicious actor needs physical access, but not much.
> For flash (SPI or NAND), at that point why doesn't the malicious actor
> just use "mw" instead? The device is in their position if they're able
> to hook up probes/etc.
> 
> So my current thought process is that yes, fixing SPI and NAND and all
> of the other forms of reading (outside of "cp") need to be fixed, as a
> follow-up series to what Sughosh is doing. And then reminding people
> that CMD_MEMORY is dangerous and perhaps think about splitting mw/etc
> out from "cmp/base/loop" so that CMD_MEMORY can be disabled for boards
> that want a more secure feel.

Pretty much new Kconfig option for production system should disable it.

> 
> I'm open to being convinced I'm wrong and this is a serious problem to
> address now, not later, however.

I have no problem with it if all of these go to 2025.01 version.

Thanks,
Michal
Simon Glass Aug. 9, 2024, 2:44 p.m. UTC | #15
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

Regards,
Simon
Michal Simek Aug. 9, 2024, 2:47 p.m. UTC | #16
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.

Thanks,
Michal
Simon Glass Aug. 9, 2024, 3:58 p.m. UTC | #17
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?

Regards,
Simon
Michal Simek Aug. 26, 2024, 8:48 a.m. UTC | #18
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
Simon Glass Sept. 1, 2024, 8:10 p.m. UTC | #19
Hi Michal,

On Mon, 26 Aug 2024 at 02:48, Michal Simek <michal.simek@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.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.

OK thanks for the info.

Regards,
Simon
diff mbox series

Patch

diff --git a/cmd/sf.c b/cmd/sf.c
index f43a2e08b3..d2ce1af8a0 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -10,6 +10,7 @@ 
 #include <div64.h>
 #include <dm.h>
 #include <log.h>
+#include <lmb.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <spi.h>
@@ -272,6 +273,35 @@  static int spi_flash_update(struct spi_flash *flash, u32 offset,
 	return 0;
 }
 
+#ifdef CONFIG_LMB
+static int do_spi_read_lmb_check(ulong start_addr, loff_t len)
+{
+	struct lmb lmb;
+	phys_size_t max_size;
+	ulong end_addr;
+
+	lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
+	lmb_dump_all(&lmb);
+
+	max_size = lmb_get_free_size(&lmb, start_addr);
+	if (!max_size) {
+		printf("ERROR: trying to overwrite reserved memory...\n");
+		return CMD_RET_FAILURE;
+	}
+
+	end_addr = start_addr + max_size;
+	if (!end_addr)
+		end_addr = ULONG_MAX;
+
+	if ((start_addr + len) > end_addr) {
+		printf("ERROR: trying to overwrite reserved memory...\n");
+		return CMD_RET_FAILURE;
+	}
+
+	return 0;
+}
+#endif
+
 static int do_spi_flash_read_write(int argc, char *const argv[])
 {
 	unsigned long addr;
@@ -315,7 +345,13 @@  static int do_spi_flash_read_write(int argc, char *const argv[])
 		ret = spi_flash_update(flash, offset, len, buf);
 	} else if (strncmp(argv[0], "read", 4) == 0 ||
 			strncmp(argv[0], "write", 5) == 0) {
-		int read;
+		int read, ret;
+
+#ifdef CONFIG_LMB
+		ret = do_spi_read_lmb_check(addr, len);
+		if (ret)
+			return ret;
+#endif
 
 		read = strncmp(argv[0], "read", 4) == 0;
 		if (read)