* [PATCH v3] cmd: sf: prevent overwriting the reserved memory
@ 2024-09-10 15:43 Prasad Kummari
2024-09-11 11:20 ` Sughosh Ganu
0 siblings, 1 reply; 7+ messages in thread
From: Prasad Kummari @ 2024-09-10 15:43 UTC (permalink / raw)
To: u-boot, git
Cc: michal.simek, venkatesh.abbarapu, padmarao.begari, git, jagan,
n-francis, d-gole, Prasad Kummari
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>
---
Changes in V3:
- Removed lmb_init_and_reserve() as part of latest LMB series.
- Error message moved to one place.
- lmb_alloc_addr() is not required because the given memory address is
being checked to ensure it is free or not.
Changes in V2:
- Rebased the code changes on top of the next branch.
UT:
Tested on Versal NET board
Versal NET> fdt print /reserved-memory
reserved-memory {
ranges;
#size-cells = <0x00000002>;
#address-cells = <0x00000002>;
tf-a {
reg = <0x00000000 0x70000000 0x00000000 0x00050000>;
no-map;
};
};
Versal NET> sf read 0x70000000 0x0 0x40
device 0 offset 0x0, size 0x40
ERROR: trying to overwrite reserved memory...
Versal NET> sf write 0x70000000 0x0 0x40
device 0 offset 0x0, size 0x40
ERROR: trying to overwrite reserved memory...
relocaddr = 0x000000007febc000
Versal NET> sf read 0x000000007febc000 0x0 0x40
device 0 offset 0x0, size 0x40
ERROR: trying to overwrite reserved memory...
Versal NET> sf write 0x000000007febc000 0x0 0x40
device 0 offset 0x0, size 0x40
ERROR: trying to overwrite reserved memory...
cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/cmd/sf.c b/cmd/sf.c
index f43a2e08b3..7bb8bcfce2 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,31 @@ 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)
+{
+ phys_size_t max_size;
+ ulong end_addr;
+
+ lmb_dump_all();
+
+ max_size = lmb_get_free_size(start_addr);
+ if (!max_size) {
+ return CMD_RET_FAILURE;
+ }
+
+ end_addr = start_addr + max_size;
+ if (!end_addr)
+ end_addr = ULONG_MAX;
+
+ if ((start_addr + len) > end_addr) {
+ return CMD_RET_FAILURE;
+ }
+
+ return 0;
+}
+#endif
+
static int do_spi_flash_read_write(int argc, char *const argv[])
{
unsigned long addr;
@@ -315,7 +341,15 @@ 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) {
+ printf("ERROR: trying to overwrite reserved memory...\n");
+ return ret;
+ }
+#endif
read = strncmp(argv[0], "read", 4) == 0;
if (read)
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3] cmd: sf: prevent overwriting the reserved memory 2024-09-10 15:43 [PATCH v3] cmd: sf: prevent overwriting the reserved memory Prasad Kummari @ 2024-09-11 11:20 ` Sughosh Ganu 2024-09-11 11:23 ` Michal Simek 0 siblings, 1 reply; 7+ messages in thread From: Sughosh Ganu @ 2024-09-11 11:20 UTC (permalink / raw) To: Prasad Kummari Cc: u-boot, git, michal.simek, venkatesh.abbarapu, padmarao.begari, git, jagan, n-francis, d-gole On Tue, 10 Sept 2024 at 21:14, 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. > > Signed-off-by: Prasad Kummari <prasad.kummari@amd.com> > --- > > Changes in V3: > - Removed lmb_init_and_reserve() as part of latest LMB series. > - Error message moved to one place. > - lmb_alloc_addr() is not required because the given memory address is > being checked to ensure it is free or not. > > Changes in V2: > - Rebased the code changes on top of the next branch. > > UT: > Tested on Versal NET board > > Versal NET> fdt print /reserved-memory > reserved-memory { > ranges; > #size-cells = <0x00000002>; > #address-cells = <0x00000002>; > tf-a { > reg = <0x00000000 0x70000000 0x00000000 0x00050000>; > no-map; > }; > }; > Versal NET> sf read 0x70000000 0x0 0x40 > device 0 offset 0x0, size 0x40 > ERROR: trying to overwrite reserved memory... > > Versal NET> sf write 0x70000000 0x0 0x40 > device 0 offset 0x0, size 0x40 > ERROR: trying to overwrite reserved memory... > > relocaddr = 0x000000007febc000 > > Versal NET> sf read 0x000000007febc000 0x0 0x40 > device 0 offset 0x0, size 0x40 > ERROR: trying to overwrite reserved memory... > > Versal NET> sf write 0x000000007febc000 0x0 0x40 > device 0 offset 0x0, size 0x40 > ERROR: trying to overwrite reserved memory... > > cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/cmd/sf.c b/cmd/sf.c > index f43a2e08b3..7bb8bcfce2 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,31 @@ 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) > +{ > + phys_size_t max_size; > + ulong end_addr; > + > + lmb_dump_all(); > + > + max_size = lmb_get_free_size(start_addr); > + if (!max_size) { > + return CMD_RET_FAILURE; > + } > + > + end_addr = start_addr + max_size; > + if (!end_addr) > + end_addr = ULONG_MAX; > + > + if ((start_addr + len) > end_addr) { > + return CMD_RET_FAILURE; > + } This is one way to get the load address, yes. But please do note that with this method, if the region of memory has already been marked as reserved, the call to lmb_get_free_size() will fail, i.e. return a value of 0. Whereas if you call lmb_alloc_addr(), and if the memory region is marked reserved with the flags set to LMB_NONE, that call will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a better approach would be to use the logic used in the fs module function that I had pointed to in my earlier email. Also, like I mentioned earlier, it will be better to refactor the functionality, especially if the plan is to introduce this check for loading stuff through other interfaces like nand, mmc etc. You can add a function in the lmb.c like lmb_read_check() which does that. -sughosh > + > + return 0; > +} > +#endif > + > static int do_spi_flash_read_write(int argc, char *const argv[]) > { > unsigned long addr; > @@ -315,7 +341,15 @@ 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) { > + printf("ERROR: trying to overwrite reserved memory...\n"); > + return ret; > + } > +#endif > > read = strncmp(argv[0], "read", 4) == 0; > if (read) > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] cmd: sf: prevent overwriting the reserved memory 2024-09-11 11:20 ` Sughosh Ganu @ 2024-09-11 11:23 ` Michal Simek 2024-09-11 11:29 ` Sughosh Ganu 0 siblings, 1 reply; 7+ messages in thread From: Michal Simek @ 2024-09-11 11:23 UTC (permalink / raw) To: Sughosh Ganu, Prasad Kummari Cc: u-boot, git, venkatesh.abbarapu, padmarao.begari, git, jagan, n-francis, d-gole On 9/11/24 13:20, Sughosh Ganu wrote: > On Tue, 10 Sept 2024 at 21:14, 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. >> >> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com> >> --- >> >> Changes in V3: >> - Removed lmb_init_and_reserve() as part of latest LMB series. >> - Error message moved to one place. >> - lmb_alloc_addr() is not required because the given memory address is >> being checked to ensure it is free or not. >> >> Changes in V2: >> - Rebased the code changes on top of the next branch. >> >> UT: >> Tested on Versal NET board >> >> Versal NET> fdt print /reserved-memory >> reserved-memory { >> ranges; >> #size-cells = <0x00000002>; >> #address-cells = <0x00000002>; >> tf-a { >> reg = <0x00000000 0x70000000 0x00000000 0x00050000>; >> no-map; >> }; >> }; >> Versal NET> sf read 0x70000000 0x0 0x40 >> device 0 offset 0x0, size 0x40 >> ERROR: trying to overwrite reserved memory... >> >> Versal NET> sf write 0x70000000 0x0 0x40 >> device 0 offset 0x0, size 0x40 >> ERROR: trying to overwrite reserved memory... >> >> relocaddr = 0x000000007febc000 >> >> Versal NET> sf read 0x000000007febc000 0x0 0x40 >> device 0 offset 0x0, size 0x40 >> ERROR: trying to overwrite reserved memory... >> >> Versal NET> sf write 0x000000007febc000 0x0 0x40 >> device 0 offset 0x0, size 0x40 >> ERROR: trying to overwrite reserved memory... >> >> cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- >> 1 file changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/cmd/sf.c b/cmd/sf.c >> index f43a2e08b3..7bb8bcfce2 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,31 @@ 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) >> +{ >> + phys_size_t max_size; >> + ulong end_addr; >> + >> + lmb_dump_all(); >> + >> + max_size = lmb_get_free_size(start_addr); >> + if (!max_size) { >> + return CMD_RET_FAILURE; >> + } >> + >> + end_addr = start_addr + max_size; >> + if (!end_addr) >> + end_addr = ULONG_MAX; >> + >> + if ((start_addr + len) > end_addr) { >> + return CMD_RET_FAILURE; >> + } > > This is one way to get the load address, yes. But please do note that > with this method, if the region of memory has already been marked as > reserved, the call to lmb_get_free_size() will fail, i.e. return a > value of 0. Whereas if you call lmb_alloc_addr(), and if the memory > region is marked reserved with the flags set to LMB_NONE, that call > will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a > better approach would be to use the logic used in the fs module > function that I had pointed to in my earlier email. > > Also, like I mentioned earlier, it will be better to refactor the > functionality, especially if the plan is to introduce this check for > loading stuff through other interfaces like nand, mmc etc. You can add > a function in the lmb.c like lmb_read_check() which does that. Is this something what you are going to work on? Thanks, Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] cmd: sf: prevent overwriting the reserved memory 2024-09-11 11:23 ` Michal Simek @ 2024-09-11 11:29 ` Sughosh Ganu 2024-09-11 11:32 ` Michal Simek 0 siblings, 1 reply; 7+ messages in thread From: Sughosh Ganu @ 2024-09-11 11:29 UTC (permalink / raw) To: Michal Simek Cc: Prasad Kummari, u-boot, git, venkatesh.abbarapu, padmarao.begari, git, jagan, n-francis, d-gole On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.simek@amd.com> wrote: > > > > On 9/11/24 13:20, Sughosh Ganu wrote: > > On Tue, 10 Sept 2024 at 21:14, 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. > >> > >> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com> > >> --- > >> > >> Changes in V3: > >> - Removed lmb_init_and_reserve() as part of latest LMB series. > >> - Error message moved to one place. > >> - lmb_alloc_addr() is not required because the given memory address is > >> being checked to ensure it is free or not. > >> > >> Changes in V2: > >> - Rebased the code changes on top of the next branch. > >> > >> UT: > >> Tested on Versal NET board > >> > >> Versal NET> fdt print /reserved-memory > >> reserved-memory { > >> ranges; > >> #size-cells = <0x00000002>; > >> #address-cells = <0x00000002>; > >> tf-a { > >> reg = <0x00000000 0x70000000 0x00000000 0x00050000>; > >> no-map; > >> }; > >> }; > >> Versal NET> sf read 0x70000000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> Versal NET> sf write 0x70000000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> relocaddr = 0x000000007febc000 > >> > >> Versal NET> sf read 0x000000007febc000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> Versal NET> sf write 0x000000007febc000 0x0 0x40 > >> device 0 offset 0x0, size 0x40 > >> ERROR: trying to overwrite reserved memory... > >> > >> cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- > >> 1 file changed, 35 insertions(+), 1 deletion(-) > >> > >> diff --git a/cmd/sf.c b/cmd/sf.c > >> index f43a2e08b3..7bb8bcfce2 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,31 @@ 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) > >> +{ > >> + phys_size_t max_size; > >> + ulong end_addr; > >> + > >> + lmb_dump_all(); > >> + > >> + max_size = lmb_get_free_size(start_addr); > >> + if (!max_size) { > >> + return CMD_RET_FAILURE; > >> + } > >> + > >> + end_addr = start_addr + max_size; > >> + if (!end_addr) > >> + end_addr = ULONG_MAX; > >> + > >> + if ((start_addr + len) > end_addr) { > >> + return CMD_RET_FAILURE; > >> + } > > > > This is one way to get the load address, yes. But please do note that > > with this method, if the region of memory has already been marked as > > reserved, the call to lmb_get_free_size() will fail, i.e. return a > > value of 0. Whereas if you call lmb_alloc_addr(), and if the memory > > region is marked reserved with the flags set to LMB_NONE, that call > > will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a > > better approach would be to use the logic used in the fs module > > function that I had pointed to in my earlier email. > > > > Also, like I mentioned earlier, it will be better to refactor the > > functionality, especially if the plan is to introduce this check for > > loading stuff through other interfaces like nand, mmc etc. You can add > > a function in the lmb.c like lmb_read_check() which does that. > > Is this something what you are going to work on? Can that not be done as part of this work? It is about having a single function in the lmb.c file, which simply does a check for lmb_alloc_addr(). It's not very complicated tbh. -sughosh ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] cmd: sf: prevent overwriting the reserved memory 2024-09-11 11:29 ` Sughosh Ganu @ 2024-09-11 11:32 ` Michal Simek 2024-09-11 11:56 ` Sughosh Ganu 0 siblings, 1 reply; 7+ messages in thread From: Michal Simek @ 2024-09-11 11:32 UTC (permalink / raw) To: Sughosh Ganu Cc: Prasad Kummari, u-boot, git, venkatesh.abbarapu, padmarao.begari, git, jagan, n-francis, d-gole On 9/11/24 13:29, Sughosh Ganu wrote: > On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.simek@amd.com> wrote: >> >> >> >> On 9/11/24 13:20, Sughosh Ganu wrote: >>> On Tue, 10 Sept 2024 at 21:14, 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. >>>> >>>> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com> >>>> --- >>>> >>>> Changes in V3: >>>> - Removed lmb_init_and_reserve() as part of latest LMB series. >>>> - Error message moved to one place. >>>> - lmb_alloc_addr() is not required because the given memory address is >>>> being checked to ensure it is free or not. >>>> >>>> Changes in V2: >>>> - Rebased the code changes on top of the next branch. >>>> >>>> UT: >>>> Tested on Versal NET board >>>> >>>> Versal NET> fdt print /reserved-memory >>>> reserved-memory { >>>> ranges; >>>> #size-cells = <0x00000002>; >>>> #address-cells = <0x00000002>; >>>> tf-a { >>>> reg = <0x00000000 0x70000000 0x00000000 0x00050000>; >>>> no-map; >>>> }; >>>> }; >>>> Versal NET> sf read 0x70000000 0x0 0x40 >>>> device 0 offset 0x0, size 0x40 >>>> ERROR: trying to overwrite reserved memory... >>>> >>>> Versal NET> sf write 0x70000000 0x0 0x40 >>>> device 0 offset 0x0, size 0x40 >>>> ERROR: trying to overwrite reserved memory... >>>> >>>> relocaddr = 0x000000007febc000 >>>> >>>> Versal NET> sf read 0x000000007febc000 0x0 0x40 >>>> device 0 offset 0x0, size 0x40 >>>> ERROR: trying to overwrite reserved memory... >>>> >>>> Versal NET> sf write 0x000000007febc000 0x0 0x40 >>>> device 0 offset 0x0, size 0x40 >>>> ERROR: trying to overwrite reserved memory... >>>> >>>> cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 35 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/cmd/sf.c b/cmd/sf.c >>>> index f43a2e08b3..7bb8bcfce2 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,31 @@ 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) >>>> +{ >>>> + phys_size_t max_size; >>>> + ulong end_addr; >>>> + >>>> + lmb_dump_all(); >>>> + >>>> + max_size = lmb_get_free_size(start_addr); >>>> + if (!max_size) { >>>> + return CMD_RET_FAILURE; >>>> + } >>>> + >>>> + end_addr = start_addr + max_size; >>>> + if (!end_addr) >>>> + end_addr = ULONG_MAX; >>>> + >>>> + if ((start_addr + len) > end_addr) { >>>> + return CMD_RET_FAILURE; >>>> + } >>> >>> This is one way to get the load address, yes. But please do note that >>> with this method, if the region of memory has already been marked as >>> reserved, the call to lmb_get_free_size() will fail, i.e. return a >>> value of 0. Whereas if you call lmb_alloc_addr(), and if the memory >>> region is marked reserved with the flags set to LMB_NONE, that call >>> will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a >>> better approach would be to use the logic used in the fs module >>> function that I had pointed to in my earlier email. >>> >>> Also, like I mentioned earlier, it will be better to refactor the >>> functionality, especially if the plan is to introduce this check for >>> loading stuff through other interfaces like nand, mmc etc. You can add >>> a function in the lmb.c like lmb_read_check() which does that. >> >> Is this something what you are going to work on? > > Can that not be done as part of this work? It is about having a single > function in the lmb.c file, which simply does a check for > lmb_alloc_addr(). It's not very complicated tbh. It is more about time spent on it. If you know how exactly it should look like it would be easier for you to put it together and Prasad can validate it on our platforms. Thanks, Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] cmd: sf: prevent overwriting the reserved memory 2024-09-11 11:32 ` Michal Simek @ 2024-09-11 11:56 ` Sughosh Ganu 2024-09-13 7:38 ` Kummari, Prasad 0 siblings, 1 reply; 7+ messages in thread From: Sughosh Ganu @ 2024-09-11 11:56 UTC (permalink / raw) To: Michal Simek Cc: Prasad Kummari, u-boot, git, venkatesh.abbarapu, padmarao.begari, git, jagan, n-francis, d-gole On Wed, 11 Sept 2024 at 17:02, Michal Simek <michal.simek@amd.com> wrote: > > > > On 9/11/24 13:29, Sughosh Ganu wrote: > > On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.simek@amd.com> wrote: > >> > >> > >> > >> On 9/11/24 13:20, Sughosh Ganu wrote: > >>> On Tue, 10 Sept 2024 at 21:14, 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. > >>>> > >>>> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com> > >>>> --- > >>>> > >>>> Changes in V3: > >>>> - Removed lmb_init_and_reserve() as part of latest LMB series. > >>>> - Error message moved to one place. > >>>> - lmb_alloc_addr() is not required because the given memory address is > >>>> being checked to ensure it is free or not. > >>>> > >>>> Changes in V2: > >>>> - Rebased the code changes on top of the next branch. > >>>> > >>>> UT: > >>>> Tested on Versal NET board > >>>> > >>>> Versal NET> fdt print /reserved-memory > >>>> reserved-memory { > >>>> ranges; > >>>> #size-cells = <0x00000002>; > >>>> #address-cells = <0x00000002>; > >>>> tf-a { > >>>> reg = <0x00000000 0x70000000 0x00000000 0x00050000>; > >>>> no-map; > >>>> }; > >>>> }; > >>>> Versal NET> sf read 0x70000000 0x0 0x40 > >>>> device 0 offset 0x0, size 0x40 > >>>> ERROR: trying to overwrite reserved memory... > >>>> > >>>> Versal NET> sf write 0x70000000 0x0 0x40 > >>>> device 0 offset 0x0, size 0x40 > >>>> ERROR: trying to overwrite reserved memory... > >>>> > >>>> relocaddr = 0x000000007febc000 > >>>> > >>>> Versal NET> sf read 0x000000007febc000 0x0 0x40 > >>>> device 0 offset 0x0, size 0x40 > >>>> ERROR: trying to overwrite reserved memory... > >>>> > >>>> Versal NET> sf write 0x000000007febc000 0x0 0x40 > >>>> device 0 offset 0x0, size 0x40 > >>>> ERROR: trying to overwrite reserved memory... > >>>> > >>>> cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 35 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/cmd/sf.c b/cmd/sf.c > >>>> index f43a2e08b3..7bb8bcfce2 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,31 @@ 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) > >>>> +{ > >>>> + phys_size_t max_size; > >>>> + ulong end_addr; > >>>> + > >>>> + lmb_dump_all(); > >>>> + > >>>> + max_size = lmb_get_free_size(start_addr); > >>>> + if (!max_size) { > >>>> + return CMD_RET_FAILURE; > >>>> + } > >>>> + > >>>> + end_addr = start_addr + max_size; > >>>> + if (!end_addr) > >>>> + end_addr = ULONG_MAX; > >>>> + > >>>> + if ((start_addr + len) > end_addr) { > >>>> + return CMD_RET_FAILURE; > >>>> + } > >>> > >>> This is one way to get the load address, yes. But please do note that > >>> with this method, if the region of memory has already been marked as > >>> reserved, the call to lmb_get_free_size() will fail, i.e. return a > >>> value of 0. Whereas if you call lmb_alloc_addr(), and if the memory > >>> region is marked reserved with the flags set to LMB_NONE, that call > >>> will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a > >>> better approach would be to use the logic used in the fs module > >>> function that I had pointed to in my earlier email. > >>> > >>> Also, like I mentioned earlier, it will be better to refactor the > >>> functionality, especially if the plan is to introduce this check for > >>> loading stuff through other interfaces like nand, mmc etc. You can add > >>> a function in the lmb.c like lmb_read_check() which does that. > >> > >> Is this something what you are going to work on? > > > > Can that not be done as part of this work? It is about having a single > > function in the lmb.c file, which simply does a check for > > lmb_alloc_addr(). It's not very complicated tbh. > > It is more about time spent on it. If you know how exactly it should look like > it would be easier for you to put it together and Prasad can validate it on our > platforms. Prasad, Can you try this change? If this works, you can use this. I will work on using this in other functions later. diff --git a/cmd/sf.c b/cmd/sf.c index 7bb8bcfce2..33d88a3531 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -343,13 +343,12 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) strncmp(argv[0], "write", 5) == 0) { int read, ret; -#ifdef CONFIG_LMB - ret = do_spi_read_lmb_check(addr, len); - if (ret) { - printf("ERROR: trying to overwrite reserved memory...\n"); - return ret; + if (CONFIG_IS_ENABLED(LMB)) { + if (lmb_read_check(addr, len)) { + printf("ERROR: trying to overwrite reserved memory...\n"); + return CMD_RET_FAILURE; + } } -#endif read = strncmp(argv[0], "read", 4) == 0; if (read) diff --git a/include/lmb.h b/include/lmb.h index 6ef03f9b63..601055a837 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -122,6 +122,11 @@ struct lmb *lmb_get(void); int lmb_push(struct lmb *store); void lmb_pop(struct lmb *store); +static inline int lmb_read_check(phys_addr_t addr, phys_size_t len) +{ + return lmb_alloc_addr(addr, len) == addr ? 0 : -1; +} + #endif /* __KERNEL__ */ #endif /* _LINUX_LMB_H */ -sughosh ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH v3] cmd: sf: prevent overwriting the reserved memory 2024-09-11 11:56 ` Sughosh Ganu @ 2024-09-13 7:38 ` Kummari, Prasad 0 siblings, 0 replies; 7+ messages in thread From: Kummari, Prasad @ 2024-09-13 7:38 UTC (permalink / raw) To: Sughosh Ganu, Simek, Michal Cc: u-boot@lists.denx.de, git (AMD-Xilinx), Abbarapu, Venkatesh, Begari, Padmarao, git@xilinx.com, jagan@amarulasolutions.com, n-francis@ti.com, d-gole@ti.com Hi Sughosh, > -----Original Message----- > From: Sughosh Ganu <sughosh.ganu@linaro.org> > Sent: Wednesday, September 11, 2024 5:26 PM > To: Simek, Michal <michal.simek@amd.com> > Cc: Kummari, Prasad <Prasad.Kummari@amd.com>; u-boot@lists.denx.de; > git (AMD-Xilinx) <git@amd.com>; Abbarapu, Venkatesh > <venkatesh.abbarapu@amd.com>; Begari, Padmarao > <Padmarao.Begari@amd.com>; git@xilinx.com; > jagan@amarulasolutions.com; n-francis@ti.com; d-gole@ti.com > Subject: Re: [PATCH v3] 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. > > > On Wed, 11 Sept 2024 at 17:02, Michal Simek <michal.simek@amd.com> wrote: > > > > > > > > On 9/11/24 13:29, Sughosh Ganu wrote: > > > On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.simek@amd.com> wrote: > > >> > > >> > > >> > > >> On 9/11/24 13:20, Sughosh Ganu wrote: > > >>> On Tue, 10 Sept 2024 at 21:14, 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. > > >>>> > > >>>> Signed-off-by: Prasad Kummari <prasad.kummari@amd.com> > > >>>> --- > > >>>> > > >>>> Changes in V3: > > >>>> - Removed lmb_init_and_reserve() as part of latest LMB series. > > >>>> - Error message moved to one place. > > >>>> - lmb_alloc_addr() is not required because the given memory address > is > > >>>> being checked to ensure it is free or not. > > >>>> > > >>>> Changes in V2: > > >>>> - Rebased the code changes on top of the next branch. > > >>>> > > >>>> UT: > > >>>> Tested on Versal NET board > > >>>> > > >>>> Versal NET> fdt print /reserved-memory reserved-memory { > > >>>> ranges; > > >>>> #size-cells = <0x00000002>; > > >>>> #address-cells = <0x00000002>; > > >>>> tf-a { > > >>>> reg = <0x00000000 0x70000000 0x00000000 0x00050000>; > > >>>> no-map; > > >>>> }; > > >>>> }; > > >>>> Versal NET> sf read 0x70000000 0x0 0x40 device 0 offset 0x0, size > > >>>> 0x40 > > >>>> ERROR: trying to overwrite reserved memory... > > >>>> > > >>>> Versal NET> sf write 0x70000000 0x0 0x40 device 0 offset 0x0, > > >>>> size 0x40 > > >>>> ERROR: trying to overwrite reserved memory... > > >>>> > > >>>> relocaddr = 0x000000007febc000 > > >>>> > > >>>> Versal NET> sf read 0x000000007febc000 0x0 0x40 device 0 offset > > >>>> 0x0, size 0x40 > > >>>> ERROR: trying to overwrite reserved memory... > > >>>> > > >>>> Versal NET> sf write 0x000000007febc000 0x0 0x40 device 0 offset > > >>>> 0x0, size 0x40 > > >>>> ERROR: trying to overwrite reserved memory... > > >>>> > > >>>> cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++- > > >>>> 1 file changed, 35 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/cmd/sf.c b/cmd/sf.c > > >>>> index f43a2e08b3..7bb8bcfce2 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,31 @@ 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) { > > >>>> + phys_size_t max_size; > > >>>> + ulong end_addr; > > >>>> + > > >>>> + lmb_dump_all(); > > >>>> + > > >>>> + max_size = lmb_get_free_size(start_addr); > > >>>> + if (!max_size) { > > >>>> + return CMD_RET_FAILURE; > > >>>> + } > > >>>> + > > >>>> + end_addr = start_addr + max_size; > > >>>> + if (!end_addr) > > >>>> + end_addr = ULONG_MAX; > > >>>> + > > >>>> + if ((start_addr + len) > end_addr) { > > >>>> + return CMD_RET_FAILURE; > > >>>> + } > > >>> > > >>> This is one way to get the load address, yes. But please do note > > >>> that with this method, if the region of memory has already been > > >>> marked as reserved, the call to lmb_get_free_size() will fail, > > >>> i.e. return a value of 0. Whereas if you call lmb_alloc_addr(), > > >>> and if the memory region is marked reserved with the flags set to > > >>> LMB_NONE, that call will not fail, as we can re-reserve memory > > >>> marked as LMB_NONE. Hence a better approach would be to use the > > >>> logic used in the fs module function that I had pointed to in my earlier > email. > > >>> > > >>> Also, like I mentioned earlier, it will be better to refactor the > > >>> functionality, especially if the plan is to introduce this check > > >>> for loading stuff through other interfaces like nand, mmc etc. You > > >>> can add a function in the lmb.c like lmb_read_check() which does that. > > >> > > >> Is this something what you are going to work on? > > > > > > Can that not be done as part of this work? It is about having a > > > single function in the lmb.c file, which simply does a check for > > > lmb_alloc_addr(). It's not very complicated tbh. > > > > It is more about time spent on it. If you know how exactly it should > > look like it would be easier for you to put it together and Prasad can > > validate it on our platforms. > > Prasad, Can you try this change? If this works, you can use this. I will work on > using this in other functions later. [Prasad]: I have tried the changes below, and they are working as expected. I have sent v4 with the latest changes. > > diff --git a/cmd/sf.c b/cmd/sf.c > index 7bb8bcfce2..33d88a3531 100644 > --- a/cmd/sf.c > +++ b/cmd/sf.c > @@ -343,13 +343,12 @@ static int do_spi_flash_read_write(int argc, char > *const argv[]) > strncmp(argv[0], "write", 5) == 0) { > int read, ret; > > -#ifdef CONFIG_LMB > - ret = do_spi_read_lmb_check(addr, len); > - if (ret) { > - printf("ERROR: trying to overwrite reserved > memory...\n"); > - return ret; > + if (CONFIG_IS_ENABLED(LMB)) { > + if (lmb_read_check(addr, len)) { > + printf("ERROR: trying to overwrite > reserved memory...\n"); > + return CMD_RET_FAILURE; > + } > } > -#endif > > read = strncmp(argv[0], "read", 4) == 0; > if (read) > diff --git a/include/lmb.h b/include/lmb.h index 6ef03f9b63..601055a837 > 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -122,6 +122,11 @@ struct lmb *lmb_get(void); int lmb_push(struct lmb > *store); void lmb_pop(struct lmb *store); > > +static inline int lmb_read_check(phys_addr_t addr, phys_size_t len) { > + return lmb_alloc_addr(addr, len) == addr ? 0 : -1; } > + > #endif /* __KERNEL__ */ > > #endif /* _LINUX_LMB_H */ > > -sughosh ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-13 7:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-10 15:43 [PATCH v3] cmd: sf: prevent overwriting the reserved memory Prasad Kummari 2024-09-11 11:20 ` Sughosh Ganu 2024-09-11 11:23 ` Michal Simek 2024-09-11 11:29 ` Sughosh Ganu 2024-09-11 11:32 ` Michal Simek 2024-09-11 11:56 ` Sughosh Ganu 2024-09-13 7:38 ` Kummari, Prasad
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.