| 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 |
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 >
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 > >
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
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
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
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.
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
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
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
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
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
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.
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
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
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
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
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
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
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 --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)
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(-)