On Wed, 11 Sept 2024 at 17:02, Michal Simek <michal.si...@amd.com> wrote: > > > > On 9/11/24 13:29, Sughosh Ganu wrote: > > On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.si...@amd.com> wrote: > >> > >> > >> > >> On 9/11/24 13:20, Sughosh Ganu wrote: > >>> On Tue, 10 Sept 2024 at 21:14, Prasad Kummari <prasad.kumm...@amd.com> > >>> wrote: > >>>> > >>>> Added LMB API to prevent SF command from overwriting reserved > >>>> memory areas. The current SPI code does not use LMB APIs for > >>>> loading data into memory addresses. To resolve this, LMB APIs > >>>> were added to check the load address of an SF command and ensure it > >>>> does not overwrite reserved memory addresses. Similar checks are > >>>> used in TFTP, serial load, and boot code to prevent overwriting > >>>> reserved memory. > >>>> > >>>> Signed-off-by: Prasad Kummari <prasad.kumm...@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