| Message ID | 20240913073251.2286529-2-prasad.kummari@amd.com |
|---|---|
| State | Accepted |
| Commit | 33a4dfc703b43f0e8d4da76411ee3f5aee0c8033 |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [v4] cmd: sf: prevent overwriting the reserved memory | expand |
On 9/13/24 09:32, 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> > --- > Changes in V4: > - Removed do_spi_read_lmb_check(). > - Added the lmb_read_check() function in lmb.c, making it reusable for > NAND, MMC, etc. > - Addressed review comments. > > 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. > > 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... > > 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... > Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > SF: 16777216 bytes @ 0x0 Erased: OK > device 0 offset 0x0, size 0x1000000 > SF: 16777216 bytes @ 0x0 Written: OK > device 0 offset 0x0, size 0x1000000 > SF: 16777216 bytes @ 0x0 Read: OK > Total of 16777216 byte(s) were the same > > cmd/sf.c | 8 ++++++++ > include/lmb.h | 5 +++++ > 2 files changed, 13 insertions(+) > > diff --git a/cmd/sf.c b/cmd/sf.c > index f43a2e08b3..08e364e191 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> > @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) > strncmp(argv[0], "write", 5) == 0) { > int read; > > + if (CONFIG_IS_ENABLED(LMB)) { > + if (lmb_read_check(addr, len)) { > + printf("ERROR: trying to overwrite reserved memory...\n"); > + return CMD_RET_FAILURE; > + } > + } > + > read = strncmp(argv[0], "read", 4) == 0; > if (read) > ret = spi_flash_read(flash, offset, len, buf); > diff --git a/include/lmb.h b/include/lmb.h > index fc2daaa7bf..aee2f9fcda 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -111,6 +111,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 */ It is coming based on discussion with Sughosh in v3 that's why Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org> Thanks, Michal
Hi Prasad, On 13/09/24 13:02, 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> > --- > Changes in V4: > - Removed do_spi_read_lmb_check(). > - Added the lmb_read_check() function in lmb.c, making it reusable for > NAND, MMC, etc. > - Addressed review comments. > > 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. > > 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... > > 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... > Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > SF: 16777216 bytes @ 0x0 Erased: OK > device 0 offset 0x0, size 0x1000000 > SF: 16777216 bytes @ 0x0 Written: OK > device 0 offset 0x0, size 0x1000000 > SF: 16777216 bytes @ 0x0 Read: OK > Total of 16777216 byte(s) were the same > > cmd/sf.c | 8 ++++++++ > include/lmb.h | 5 +++++ > 2 files changed, 13 insertions(+) > > diff --git a/cmd/sf.c b/cmd/sf.c > index f43a2e08b3..08e364e191 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> > @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) > strncmp(argv[0], "write", 5) == 0) { > int read; > > + if (CONFIG_IS_ENABLED(LMB)) { > + if (lmb_read_check(addr, len)) { Even though the function is named lmb_read_check(), it performs an alloc which is never freed, thus it makes it difficult for other callers to use the same region for other purposes (some callers use lmb_get_free_size() ), as mentioned in the commit message the check is only to prevent sf from overwriting reserved region, but it looks like this patch makes the load region also as reserved, is this necessary? > + printf("ERROR: trying to overwrite reserved memory...\n"); > + return CMD_RET_FAILURE; > + } > + } > + > read = strncmp(argv[0], "read", 4) == 0; > if (read) > ret = spi_flash_read(flash, offset, len, buf); > diff --git a/include/lmb.h b/include/lmb.h > index fc2daaa7bf..aee2f9fcda 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -111,6 +111,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; who frees this? can we free this right after checking? Thanks and Regards, Vaishnav > +} > + > #endif /* __KERNEL__ */ > > #endif /* _LINUX_LMB_H */
On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: > > Hi Prasad, > > On 13/09/24 13:02, 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> > > --- > > Changes in V4: > > - Removed do_spi_read_lmb_check(). > > - Added the lmb_read_check() function in lmb.c, making it reusable for > > NAND, MMC, etc. > > - Addressed review comments. > > > > 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. > > > > 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... > > > > 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... > > Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > > write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > > 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > > SF: 16777216 bytes @ 0x0 Erased: OK > > device 0 offset 0x0, size 0x1000000 > > SF: 16777216 bytes @ 0x0 Written: OK > > device 0 offset 0x0, size 0x1000000 > > SF: 16777216 bytes @ 0x0 Read: OK > > Total of 16777216 byte(s) were the same > > > > cmd/sf.c | 8 ++++++++ > > include/lmb.h | 5 +++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/cmd/sf.c b/cmd/sf.c > > index f43a2e08b3..08e364e191 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> > > @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) > > strncmp(argv[0], "write", 5) == 0) { > > int read; > > > > + if (CONFIG_IS_ENABLED(LMB)) { > > + if (lmb_read_check(addr, len)) { > > Even though the function is named lmb_read_check(), it performs an alloc > which is never freed, thus it makes it difficult for other callers to > use the same region for other purposes (some callers use > lmb_get_free_size() ), as mentioned in the commit message the check is > only to prevent sf from overwriting reserved region, but it looks like > this patch makes the load region also as reserved, is this necessary? Like I mentioned in my other reply, using a check for lmb_alloc_addr() allows for memory re-use, which is the behaviour that a large number of use cases rely on -- if you go through the test scripts, it is assumed that memory re-use is allowed. That there is no need to explicitly free up memory, and that has been how the LMB memory has been used historically. So it is allowed to use some address to load an image to that address, and then use the same address to load a different image. The LMB rework series does keep this behaviour consistent. So it would be better to change the behaviour of the tftp command to use the same API. I was planning on working on this cleanup. If you want, you can take it up. > > > > + printf("ERROR: trying to overwrite reserved memory...\n"); > > + return CMD_RET_FAILURE; > > + } > > + } > > + > > read = strncmp(argv[0], "read", 4) == 0; > > if (read) > > ret = spi_flash_read(flash, offset, len, buf); > > diff --git a/include/lmb.h b/include/lmb.h > > index fc2daaa7bf..aee2f9fcda 100644 > > --- a/include/lmb.h > > +++ b/include/lmb.h > > @@ -111,6 +111,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; > > who frees this? can we free this right after checking? It is not required to explicitly free up this memory, as it is not an actual allocation per-se. Why were these functions called alloc something, I am not sure. But the point is, if you change the tftp command code to use this API instead, and then use it after a previous load, it would not fail. -sughosh > > Thanks and Regards, > Vaishnav > > > +} > > + > > #endif /* __KERNEL__ */ > > > > #endif /* _LINUX_LMB_H */
Hi Sughosh, On 16/09/24 12:13, Sughosh Ganu wrote: > On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: >> >> Hi Prasad, >> >> On 13/09/24 13:02, 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> >>> --- >>> Changes in V4: >>> - Removed do_spi_read_lmb_check(). >>> - Added the lmb_read_check() function in lmb.c, making it reusable for >>> NAND, MMC, etc. >>> - Addressed review comments. >>> >>> 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. >>> >>> 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... >>> >>> 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... >>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf >>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read >>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 >>> SF: 16777216 bytes @ 0x0 Erased: OK >>> device 0 offset 0x0, size 0x1000000 >>> SF: 16777216 bytes @ 0x0 Written: OK >>> device 0 offset 0x0, size 0x1000000 >>> SF: 16777216 bytes @ 0x0 Read: OK >>> Total of 16777216 byte(s) were the same >>> >>> cmd/sf.c | 8 ++++++++ >>> include/lmb.h | 5 +++++ >>> 2 files changed, 13 insertions(+) >>> >>> diff --git a/cmd/sf.c b/cmd/sf.c >>> index f43a2e08b3..08e364e191 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> >>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) >>> strncmp(argv[0], "write", 5) == 0) { >>> int read; >>> >>> + if (CONFIG_IS_ENABLED(LMB)) { >>> + if (lmb_read_check(addr, len)) { >> >> Even though the function is named lmb_read_check(), it performs an alloc >> which is never freed, thus it makes it difficult for other callers to >> use the same region for other purposes (some callers use >> lmb_get_free_size() ), as mentioned in the commit message the check is >> only to prevent sf from overwriting reserved region, but it looks like >> this patch makes the load region also as reserved, is this necessary? > > Like I mentioned in my other reply, using a check for lmb_alloc_addr() > allows for memory re-use, which is the behaviour that a large number > of use cases rely on -- if you go through the test scripts, it is > assumed that memory re-use is allowed. That there is no need to > explicitly free up memory, and that has been how the LMB memory has > been used historically. So it is allowed to use some address to load > an image to that address, and then use the same address to load a > different image. The LMB rework series does keep this behaviour > consistent. So it would be better to change the behaviour of the tftp > command to use the same API. I was planning on working on this > cleanup. If you want, you can take it up. > Please let me know if you are planning to work on this in the coming few days, otherwise I can pick it up as we have platforms failing due to this. >> >> >>> + printf("ERROR: trying to overwrite reserved memory...\n"); >>> + return CMD_RET_FAILURE; >>> + } >>> + } >>> + >>> read = strncmp(argv[0], "read", 4) == 0; >>> if (read) >>> ret = spi_flash_read(flash, offset, len, buf); >>> diff --git a/include/lmb.h b/include/lmb.h >>> index fc2daaa7bf..aee2f9fcda 100644 >>> --- a/include/lmb.h >>> +++ b/include/lmb.h >>> @@ -111,6 +111,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; >> >> who frees this? can we free this right after checking? > > It is not required to explicitly free up this memory, as it is not an > actual allocation per-se. Why were these functions called alloc > something, I am not sure. But the point is, if you change the tftp > command code to use this API instead, and then use it after a previous > load, it would not fail. > > -sughosh > Agreed, but the commit message says to "ensure it does not overwrite reserved memory addresses" but the implementation marks the region as reserved in the global memory map and is visible in lmb_dump_all() , which is not expected, is there really a need to mark the region as reserved. Thanks and Regards, Vaishnav >> >> Thanks and Regards, >> Vaishnav >> >>> +} >>> + >>> #endif /* __KERNEL__ */ >>> >>> #endif /* _LINUX_LMB_H */
On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishnav.a@ti.com> wrote: > > Hi Sughosh, > > On 16/09/24 12:13, Sughosh Ganu wrote: > > On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: > >> > >> Hi Prasad, > >> > >> On 13/09/24 13:02, 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> > >>> --- > >>> Changes in V4: > >>> - Removed do_spi_read_lmb_check(). > >>> - Added the lmb_read_check() function in lmb.c, making it reusable for > >>> NAND, MMC, etc. > >>> - Addressed review comments. > >>> > >>> 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. > >>> > >>> 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... > >>> > >>> 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... > >>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > >>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > >>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > >>> SF: 16777216 bytes @ 0x0 Erased: OK > >>> device 0 offset 0x0, size 0x1000000 > >>> SF: 16777216 bytes @ 0x0 Written: OK > >>> device 0 offset 0x0, size 0x1000000 > >>> SF: 16777216 bytes @ 0x0 Read: OK > >>> Total of 16777216 byte(s) were the same > >>> > >>> cmd/sf.c | 8 ++++++++ > >>> include/lmb.h | 5 +++++ > >>> 2 files changed, 13 insertions(+) > >>> > >>> diff --git a/cmd/sf.c b/cmd/sf.c > >>> index f43a2e08b3..08e364e191 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> > >>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) > >>> strncmp(argv[0], "write", 5) == 0) { > >>> int read; > >>> > >>> + if (CONFIG_IS_ENABLED(LMB)) { > >>> + if (lmb_read_check(addr, len)) { > >> > >> Even though the function is named lmb_read_check(), it performs an alloc > >> which is never freed, thus it makes it difficult for other callers to > >> use the same region for other purposes (some callers use > >> lmb_get_free_size() ), as mentioned in the commit message the check is > >> only to prevent sf from overwriting reserved region, but it looks like > >> this patch makes the load region also as reserved, is this necessary? > > > > Like I mentioned in my other reply, using a check for lmb_alloc_addr() > > allows for memory re-use, which is the behaviour that a large number > > of use cases rely on -- if you go through the test scripts, it is > > assumed that memory re-use is allowed. That there is no need to > > explicitly free up memory, and that has been how the LMB memory has > > been used historically. So it is allowed to use some address to load > > an image to that address, and then use the same address to load a > > different image. The LMB rework series does keep this behaviour > > consistent. So it would be better to change the behaviour of the tftp > > command to use the same API. I was planning on working on this > > cleanup. If you want, you can take it up. > > > > Please let me know if you are planning to work on this in the coming few > days, otherwise I can pick it up as we have platforms failing due to this. I can take this up. Will keep you on Cc so that you can test the patches on your boards. > > >> > >> > >>> + printf("ERROR: trying to overwrite reserved memory...\n"); > >>> + return CMD_RET_FAILURE; > >>> + } > >>> + } > >>> + > >>> read = strncmp(argv[0], "read", 4) == 0; > >>> if (read) > >>> ret = spi_flash_read(flash, offset, len, buf); > >>> diff --git a/include/lmb.h b/include/lmb.h > >>> index fc2daaa7bf..aee2f9fcda 100644 > >>> --- a/include/lmb.h > >>> +++ b/include/lmb.h > >>> @@ -111,6 +111,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; > >> > >> who frees this? can we free this right after checking? > > > > It is not required to explicitly free up this memory, as it is not an > > actual allocation per-se. Why were these functions called alloc > > something, I am not sure. But the point is, if you change the tftp > > command code to use this API instead, and then use it after a previous > > load, it would not fail. > > > > -sughosh > > > > Agreed, but the commit message says to "ensure it does not overwrite > reserved memory addresses" but the implementation marks the region as > reserved in the global memory map and is visible in lmb_dump_all() , > which is not expected, is there really a need to mark the region as > reserved. What can be overwritten, and what cannot be can now be determined from the flags. If you check the LMB memory map from the bdinfo command, you will see that the regions which cannot be overwritten are now being marked with the "no-overwrite" flag. The other LMB memory which can be re-used to load multiple different images is being marked with the "none" flag. One issue with all this is that currently there is no document which explains all these concepts. I will work on adding such a document. Thanks. -sughosh > > Thanks and Regards, > Vaishnav > > >> > >> Thanks and Regards, > >> Vaishnav > >> > >>> +} > >>> + > >>> #endif /* __KERNEL__ */ > >>> > >>> #endif /* _LINUX_LMB_H */
Hi Sughosh, On 16/09/24 14:53, Sughosh Ganu wrote: > On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishnav.a@ti.com> wrote: >> >> Hi Sughosh, >> >> On 16/09/24 12:13, Sughosh Ganu wrote: >>> On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: >>>> >>>> Hi Prasad, >>>> >>>> On 13/09/24 13:02, 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> >>>>> --- >>>>> Changes in V4: >>>>> - Removed do_spi_read_lmb_check(). >>>>> - Added the lmb_read_check() function in lmb.c, making it reusable for >>>>> NAND, MMC, etc. >>>>> - Addressed review comments. >>>>> >>>>> 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. >>>>> >>>>> 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... >>>>> >>>>> 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... >>>>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf >>>>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read >>>>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 >>>>> SF: 16777216 bytes @ 0x0 Erased: OK >>>>> device 0 offset 0x0, size 0x1000000 >>>>> SF: 16777216 bytes @ 0x0 Written: OK >>>>> device 0 offset 0x0, size 0x1000000 >>>>> SF: 16777216 bytes @ 0x0 Read: OK >>>>> Total of 16777216 byte(s) were the same >>>>> >>>>> cmd/sf.c | 8 ++++++++ >>>>> include/lmb.h | 5 +++++ >>>>> 2 files changed, 13 insertions(+) >>>>> >>>>> diff --git a/cmd/sf.c b/cmd/sf.c >>>>> index f43a2e08b3..08e364e191 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> >>>>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) >>>>> strncmp(argv[0], "write", 5) == 0) { >>>>> int read; >>>>> >>>>> + if (CONFIG_IS_ENABLED(LMB)) { >>>>> + if (lmb_read_check(addr, len)) { >>>> >>>> Even though the function is named lmb_read_check(), it performs an alloc >>>> which is never freed, thus it makes it difficult for other callers to >>>> use the same region for other purposes (some callers use >>>> lmb_get_free_size() ), as mentioned in the commit message the check is >>>> only to prevent sf from overwriting reserved region, but it looks like >>>> this patch makes the load region also as reserved, is this necessary? >>> >>> Like I mentioned in my other reply, using a check for lmb_alloc_addr() >>> allows for memory re-use, which is the behaviour that a large number >>> of use cases rely on -- if you go through the test scripts, it is >>> assumed that memory re-use is allowed. That there is no need to >>> explicitly free up memory, and that has been how the LMB memory has >>> been used historically. So it is allowed to use some address to load >>> an image to that address, and then use the same address to load a >>> different image. The LMB rework series does keep this behaviour >>> consistent. So it would be better to change the behaviour of the tftp >>> command to use the same API. I was planning on working on this >>> cleanup. If you want, you can take it up. >>> >> >> Please let me know if you are planning to work on this in the coming few >> days, otherwise I can pick it up as we have platforms failing due to this. > > I can take this up. Will keep you on Cc so that you can test the > patches on your boards. > Thanks, I will test and report the results once you post the patches. >> >>>> >>>> >>>>> + printf("ERROR: trying to overwrite reserved memory...\n"); >>>>> + return CMD_RET_FAILURE; >>>>> + } >>>>> + } >>>>> + >>>>> read = strncmp(argv[0], "read", 4) == 0; >>>>> if (read) >>>>> ret = spi_flash_read(flash, offset, len, buf); >>>>> diff --git a/include/lmb.h b/include/lmb.h >>>>> index fc2daaa7bf..aee2f9fcda 100644 >>>>> --- a/include/lmb.h >>>>> +++ b/include/lmb.h >>>>> @@ -111,6 +111,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; >>>> >>>> who frees this? can we free this right after checking? >>> >>> It is not required to explicitly free up this memory, as it is not an >>> actual allocation per-se. Why were these functions called alloc >>> something, I am not sure. But the point is, if you change the tftp >>> command code to use this API instead, and then use it after a previous >>> load, it would not fail. >>> >>> -sughosh >>> >> >> Agreed, but the commit message says to "ensure it does not overwrite >> reserved memory addresses" but the implementation marks the region as >> reserved in the global memory map and is visible in lmb_dump_all() , >> which is not expected, is there really a need to mark the region as >> reserved. > > What can be overwritten, and what cannot be can now be determined from > the flags. If you check the LMB memory map from the bdinfo command, > you will see that the regions which cannot be overwritten are now > being marked with the "no-overwrite" flag. The other LMB memory which > can be re-used to load multiple different images is being marked with > the "none" flag. One issue with all this is that currently there is no > document which explains all these concepts. I will work on adding such > a document. Thanks. > Yes, documenting the behavior will clear the confusion. Thanks and Regards, Vaishnav > -sughosh > >> >> Thanks and Regards, >> Vaishnav >> >>>> >>>> Thanks and Regards, >>>> Vaishnav >>>> >>>>> +} >>>>> + >>>>> #endif /* __KERNEL__ */ >>>>> >>>>> #endif /* _LINUX_LMB_H */
On Mon, 16 Sept 2024 at 16:07, Vaishnav Achath <vaishnav.a@ti.com> wrote: > > Hi Sughosh, > > On 16/09/24 14:53, Sughosh Ganu wrote: > > On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishnav.a@ti.com> wrote: > >> > >> Hi Sughosh, > >> > >> On 16/09/24 12:13, Sughosh Ganu wrote: > >>> On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: > >>>> > >>>> Hi Prasad, > >>>> > >>>> On 13/09/24 13:02, 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> > >>>>> --- > >>>>> Changes in V4: > >>>>> - Removed do_spi_read_lmb_check(). > >>>>> - Added the lmb_read_check() function in lmb.c, making it reusable for > >>>>> NAND, MMC, etc. > >>>>> - Addressed review comments. > >>>>> > >>>>> 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. > >>>>> > >>>>> 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... > >>>>> > >>>>> 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... > >>>>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > >>>>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > >>>>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > >>>>> SF: 16777216 bytes @ 0x0 Erased: OK > >>>>> device 0 offset 0x0, size 0x1000000 > >>>>> SF: 16777216 bytes @ 0x0 Written: OK > >>>>> device 0 offset 0x0, size 0x1000000 > >>>>> SF: 16777216 bytes @ 0x0 Read: OK > >>>>> Total of 16777216 byte(s) were the same > >>>>> > >>>>> cmd/sf.c | 8 ++++++++ > >>>>> include/lmb.h | 5 +++++ > >>>>> 2 files changed, 13 insertions(+) > >>>>> > >>>>> diff --git a/cmd/sf.c b/cmd/sf.c > >>>>> index f43a2e08b3..08e364e191 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> > >>>>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) > >>>>> strncmp(argv[0], "write", 5) == 0) { > >>>>> int read; > >>>>> > >>>>> + if (CONFIG_IS_ENABLED(LMB)) { > >>>>> + if (lmb_read_check(addr, len)) { > >>>> > >>>> Even though the function is named lmb_read_check(), it performs an alloc > >>>> which is never freed, thus it makes it difficult for other callers to > >>>> use the same region for other purposes (some callers use > >>>> lmb_get_free_size() ), as mentioned in the commit message the check is > >>>> only to prevent sf from overwriting reserved region, but it looks like > >>>> this patch makes the load region also as reserved, is this necessary? > >>> > >>> Like I mentioned in my other reply, using a check for lmb_alloc_addr() > >>> allows for memory re-use, which is the behaviour that a large number > >>> of use cases rely on -- if you go through the test scripts, it is > >>> assumed that memory re-use is allowed. That there is no need to > >>> explicitly free up memory, and that has been how the LMB memory has > >>> been used historically. So it is allowed to use some address to load > >>> an image to that address, and then use the same address to load a > >>> different image. The LMB rework series does keep this behaviour > >>> consistent. So it would be better to change the behaviour of the tftp > >>> command to use the same API. I was planning on working on this > >>> cleanup. If you want, you can take it up. > >>> > >> > >> Please let me know if you are planning to work on this in the coming few > >> days, otherwise I can pick it up as we have platforms failing due to this. > > > > I can take this up. Will keep you on Cc so that you can test the > > patches on your boards. > > > > Thanks, I will test and report the results once you post the patches. I have pushed a couple of patches to my github branch [1]. Can you please try these on your platforms and check if this fixes the issues that you see. I will also set up a board with network access on my end and try it out. Thanks. -sughosh [1] - https://github.com/sughoshg/u-boot/tree/tftp_wget_lmb_changes > > >> > >>>> > >>>> > >>>>> + printf("ERROR: trying to overwrite reserved memory...\n"); > >>>>> + return CMD_RET_FAILURE; > >>>>> + } > >>>>> + } > >>>>> + > >>>>> read = strncmp(argv[0], "read", 4) == 0; > >>>>> if (read) > >>>>> ret = spi_flash_read(flash, offset, len, buf); > >>>>> diff --git a/include/lmb.h b/include/lmb.h > >>>>> index fc2daaa7bf..aee2f9fcda 100644 > >>>>> --- a/include/lmb.h > >>>>> +++ b/include/lmb.h > >>>>> @@ -111,6 +111,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; > >>>> > >>>> who frees this? can we free this right after checking? > >>> > >>> It is not required to explicitly free up this memory, as it is not an > >>> actual allocation per-se. Why were these functions called alloc > >>> something, I am not sure. But the point is, if you change the tftp > >>> command code to use this API instead, and then use it after a previous > >>> load, it would not fail. > >>> > >>> -sughosh > >>> > >> > >> Agreed, but the commit message says to "ensure it does not overwrite > >> reserved memory addresses" but the implementation marks the region as > >> reserved in the global memory map and is visible in lmb_dump_all() , > >> which is not expected, is there really a need to mark the region as > >> reserved. > > > > What can be overwritten, and what cannot be can now be determined from > > the flags. If you check the LMB memory map from the bdinfo command, > > you will see that the regions which cannot be overwritten are now > > being marked with the "no-overwrite" flag. The other LMB memory which > > can be re-used to load multiple different images is being marked with > > the "none" flag. One issue with all this is that currently there is no > > document which explains all these concepts. I will work on adding such > > a document. Thanks. > > > > Yes, documenting the behavior will clear the confusion. > > Thanks and Regards, > Vaishnav > > > -sughosh > > > >> > >> Thanks and Regards, > >> Vaishnav > >> > >>>> > >>>> Thanks and Regards, > >>>> Vaishnav > >>>> > >>>>> +} > >>>>> + > >>>>> #endif /* __KERNEL__ */ > >>>>> > >>>>> #endif /* _LINUX_LMB_H */
Hi Sughosh, On 16/09/24 16:40, Sughosh Ganu wrote: > On Mon, 16 Sept 2024 at 16:07, Vaishnav Achath <vaishnav.a@ti.com> wrote: >> >> Hi Sughosh, >> >> On 16/09/24 14:53, Sughosh Ganu wrote: >>> On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishnav.a@ti.com> wrote: >>>> >>>> Hi Sughosh, >>>> >>>> On 16/09/24 12:13, Sughosh Ganu wrote: >>>>> On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: >>>>>> >>>>>> Hi Prasad, >>>>>> >>>>>> On 13/09/24 13:02, 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> >>>>>>> --- >>>>>>> Changes in V4: >>>>>>> - Removed do_spi_read_lmb_check(). >>>>>>> - Added the lmb_read_check() function in lmb.c, making it reusable for >>>>>>> NAND, MMC, etc. >>>>>>> - Addressed review comments. >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> 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... >>>>>>> >>>>>>> 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... >>>>>>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf >>>>>>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read >>>>>>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 >>>>>>> SF: 16777216 bytes @ 0x0 Erased: OK >>>>>>> device 0 offset 0x0, size 0x1000000 >>>>>>> SF: 16777216 bytes @ 0x0 Written: OK >>>>>>> device 0 offset 0x0, size 0x1000000 >>>>>>> SF: 16777216 bytes @ 0x0 Read: OK >>>>>>> Total of 16777216 byte(s) were the same >>>>>>> >>>>>>> cmd/sf.c | 8 ++++++++ >>>>>>> include/lmb.h | 5 +++++ >>>>>>> 2 files changed, 13 insertions(+) >>>>>>> >>>>>>> diff --git a/cmd/sf.c b/cmd/sf.c >>>>>>> index f43a2e08b3..08e364e191 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> >>>>>>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) >>>>>>> strncmp(argv[0], "write", 5) == 0) { >>>>>>> int read; >>>>>>> >>>>>>> + if (CONFIG_IS_ENABLED(LMB)) { >>>>>>> + if (lmb_read_check(addr, len)) { >>>>>> >>>>>> Even though the function is named lmb_read_check(), it performs an alloc >>>>>> which is never freed, thus it makes it difficult for other callers to >>>>>> use the same region for other purposes (some callers use >>>>>> lmb_get_free_size() ), as mentioned in the commit message the check is >>>>>> only to prevent sf from overwriting reserved region, but it looks like >>>>>> this patch makes the load region also as reserved, is this necessary? >>>>> >>>>> Like I mentioned in my other reply, using a check for lmb_alloc_addr() >>>>> allows for memory re-use, which is the behaviour that a large number >>>>> of use cases rely on -- if you go through the test scripts, it is >>>>> assumed that memory re-use is allowed. That there is no need to >>>>> explicitly free up memory, and that has been how the LMB memory has >>>>> been used historically. So it is allowed to use some address to load >>>>> an image to that address, and then use the same address to load a >>>>> different image. The LMB rework series does keep this behaviour >>>>> consistent. So it would be better to change the behaviour of the tftp >>>>> command to use the same API. I was planning on working on this >>>>> cleanup. If you want, you can take it up. >>>>> >>>> >>>> Please let me know if you are planning to work on this in the coming few >>>> days, otherwise I can pick it up as we have platforms failing due to this. >>> >>> I can take this up. Will keep you on Cc so that you can test the >>> patches on your boards. >>> >> >> Thanks, I will test and report the results once you post the patches. > > I have pushed a couple of patches to my github branch [1]. Can you > please try these on your platforms and check if this fixes the issues > that you see. I will also set up a board with network access on my end > and try it out. Thanks. > > -sughosh > > [1] - https://github.com/sughoshg/u-boot/tree/tftp_wget_lmb_changes > I tested with your patches on our failing platforms and it is working, Thanks, In the logs, initially a mmc load of remoteproc firmware happens and then Kernel/DTB load through TFTP which was failing, now it is fixed with your patches: https://gist.github.com/vachath/bb7c8c7b5a38a58298026b831db58424 Thanks and Regards, Vaishnav >> >>>> >>>>>> >>>>>> >>>>>>> + printf("ERROR: trying to overwrite reserved memory...\n"); >>>>>>> + return CMD_RET_FAILURE; >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> read = strncmp(argv[0], "read", 4) == 0; >>>>>>> if (read) >>>>>>> ret = spi_flash_read(flash, offset, len, buf); >>>>>>> diff --git a/include/lmb.h b/include/lmb.h >>>>>>> index fc2daaa7bf..aee2f9fcda 100644 >>>>>>> --- a/include/lmb.h >>>>>>> +++ b/include/lmb.h >>>>>>> @@ -111,6 +111,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; >>>>>> >>>>>> who frees this? can we free this right after checking? >>>>> >>>>> It is not required to explicitly free up this memory, as it is not an >>>>> actual allocation per-se. Why were these functions called alloc >>>>> something, I am not sure. But the point is, if you change the tftp >>>>> command code to use this API instead, and then use it after a previous >>>>> load, it would not fail. >>>>> >>>>> -sughosh >>>>> >>>> >>>> Agreed, but the commit message says to "ensure it does not overwrite >>>> reserved memory addresses" but the implementation marks the region as >>>> reserved in the global memory map and is visible in lmb_dump_all() , >>>> which is not expected, is there really a need to mark the region as >>>> reserved. >>> >>> What can be overwritten, and what cannot be can now be determined from >>> the flags. If you check the LMB memory map from the bdinfo command, >>> you will see that the regions which cannot be overwritten are now >>> being marked with the "no-overwrite" flag. The other LMB memory which >>> can be re-used to load multiple different images is being marked with >>> the "none" flag. One issue with all this is that currently there is no >>> document which explains all these concepts. I will work on adding such >>> a document. Thanks. >>> >> >> Yes, documenting the behavior will clear the confusion. >> >> Thanks and Regards, >> Vaishnav >> >>> -sughosh >>> >>>> >>>> Thanks and Regards, >>>> Vaishnav >>>> >>>>>> >>>>>> Thanks and Regards, >>>>>> Vaishnav >>>>>> >>>>>>> +} >>>>>>> + >>>>>>> #endif /* __KERNEL__ */ >>>>>>> >>>>>>> #endif /* _LINUX_LMB_H */
On Mon, 16 Sept 2024 at 19:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: > > Hi Sughosh, > > On 16/09/24 16:40, Sughosh Ganu wrote: > > On Mon, 16 Sept 2024 at 16:07, Vaishnav Achath <vaishnav.a@ti.com> wrote: > >> > >> Hi Sughosh, > >> > >> On 16/09/24 14:53, Sughosh Ganu wrote: > >>> On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishnav.a@ti.com> wrote: > >>>> > >>>> Hi Sughosh, > >>>> > >>>> On 16/09/24 12:13, Sughosh Ganu wrote: > >>>>> On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a@ti.com> wrote: > >>>>>> > >>>>>> Hi Prasad, > >>>>>> > >>>>>> On 13/09/24 13:02, 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> > >>>>>>> --- > >>>>>>> Changes in V4: > >>>>>>> - Removed do_spi_read_lmb_check(). > >>>>>>> - Added the lmb_read_check() function in lmb.c, making it reusable for > >>>>>>> NAND, MMC, etc. > >>>>>>> - Addressed review comments. > >>>>>>> > >>>>>>> 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. > >>>>>>> > >>>>>>> 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... > >>>>>>> > >>>>>>> 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... > >>>>>>> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf > >>>>>>> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read > >>>>>>> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 > >>>>>>> SF: 16777216 bytes @ 0x0 Erased: OK > >>>>>>> device 0 offset 0x0, size 0x1000000 > >>>>>>> SF: 16777216 bytes @ 0x0 Written: OK > >>>>>>> device 0 offset 0x0, size 0x1000000 > >>>>>>> SF: 16777216 bytes @ 0x0 Read: OK > >>>>>>> Total of 16777216 byte(s) were the same > >>>>>>> > >>>>>>> cmd/sf.c | 8 ++++++++ > >>>>>>> include/lmb.h | 5 +++++ > >>>>>>> 2 files changed, 13 insertions(+) > >>>>>>> > >>>>>>> diff --git a/cmd/sf.c b/cmd/sf.c > >>>>>>> index f43a2e08b3..08e364e191 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> > >>>>>>> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) > >>>>>>> strncmp(argv[0], "write", 5) == 0) { > >>>>>>> int read; > >>>>>>> > >>>>>>> + if (CONFIG_IS_ENABLED(LMB)) { > >>>>>>> + if (lmb_read_check(addr, len)) { > >>>>>> > >>>>>> Even though the function is named lmb_read_check(), it performs an alloc > >>>>>> which is never freed, thus it makes it difficult for other callers to > >>>>>> use the same region for other purposes (some callers use > >>>>>> lmb_get_free_size() ), as mentioned in the commit message the check is > >>>>>> only to prevent sf from overwriting reserved region, but it looks like > >>>>>> this patch makes the load region also as reserved, is this necessary? > >>>>> > >>>>> Like I mentioned in my other reply, using a check for lmb_alloc_addr() > >>>>> allows for memory re-use, which is the behaviour that a large number > >>>>> of use cases rely on -- if you go through the test scripts, it is > >>>>> assumed that memory re-use is allowed. That there is no need to > >>>>> explicitly free up memory, and that has been how the LMB memory has > >>>>> been used historically. So it is allowed to use some address to load > >>>>> an image to that address, and then use the same address to load a > >>>>> different image. The LMB rework series does keep this behaviour > >>>>> consistent. So it would be better to change the behaviour of the tftp > >>>>> command to use the same API. I was planning on working on this > >>>>> cleanup. If you want, you can take it up. > >>>>> > >>>> > >>>> Please let me know if you are planning to work on this in the coming few > >>>> days, otherwise I can pick it up as we have platforms failing due to this. > >>> > >>> I can take this up. Will keep you on Cc so that you can test the > >>> patches on your boards. > >>> > >> > >> Thanks, I will test and report the results once you post the patches. > > > > I have pushed a couple of patches to my github branch [1]. Can you > > please try these on your platforms and check if this fixes the issues > > that you see. I will also set up a board with network access on my end > > and try it out. Thanks. > > > > -sughosh > > > > [1] - https://github.com/sughoshg/u-boot/tree/tftp_wget_lmb_changes > > > > I tested with your patches on our failing platforms and it is working, > Thanks, In the logs, initially a mmc load of remoteproc firmware happens > and then Kernel/DTB load through TFTP which was failing, now it is fixed > with your patches: Thanks much for testing the patches! I have put the patches through CI. Will post them on the list once the CI completes. -sughosh > > https://gist.github.com/vachath/bb7c8c7b5a38a58298026b831db58424 > > Thanks and Regards, > Vaishnav > > >> > >>>> > >>>>>> > >>>>>> > >>>>>>> + printf("ERROR: trying to overwrite reserved memory...\n"); > >>>>>>> + return CMD_RET_FAILURE; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + > >>>>>>> read = strncmp(argv[0], "read", 4) == 0; > >>>>>>> if (read) > >>>>>>> ret = spi_flash_read(flash, offset, len, buf); > >>>>>>> diff --git a/include/lmb.h b/include/lmb.h > >>>>>>> index fc2daaa7bf..aee2f9fcda 100644 > >>>>>>> --- a/include/lmb.h > >>>>>>> +++ b/include/lmb.h > >>>>>>> @@ -111,6 +111,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; > >>>>>> > >>>>>> who frees this? can we free this right after checking? > >>>>> > >>>>> It is not required to explicitly free up this memory, as it is not an > >>>>> actual allocation per-se. Why were these functions called alloc > >>>>> something, I am not sure. But the point is, if you change the tftp > >>>>> command code to use this API instead, and then use it after a previous > >>>>> load, it would not fail. > >>>>> > >>>>> -sughosh > >>>>> > >>>> > >>>> Agreed, but the commit message says to "ensure it does not overwrite > >>>> reserved memory addresses" but the implementation marks the region as > >>>> reserved in the global memory map and is visible in lmb_dump_all() , > >>>> which is not expected, is there really a need to mark the region as > >>>> reserved. > >>> > >>> What can be overwritten, and what cannot be can now be determined from > >>> the flags. If you check the LMB memory map from the bdinfo command, > >>> you will see that the regions which cannot be overwritten are now > >>> being marked with the "no-overwrite" flag. The other LMB memory which > >>> can be re-used to load multiple different images is being marked with > >>> the "none" flag. One issue with all this is that currently there is no > >>> document which explains all these concepts. I will work on adding such > >>> a document. Thanks. > >>> > >> > >> Yes, documenting the behavior will clear the confusion. > >> > >> Thanks and Regards, > >> Vaishnav > >> > >>> -sughosh > >>> > >>>> > >>>> Thanks and Regards, > >>>> Vaishnav > >>>> > >>>>>> > >>>>>> Thanks and Regards, > >>>>>> Vaishnav > >>>>>> > >>>>>>> +} > >>>>>>> + > >>>>>>> #endif /* __KERNEL__ */ > >>>>>>> > >>>>>>> #endif /* _LINUX_LMB_H */
On Fri, 13 Sep 2024 13:02:52 +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. > > [...] Applied to u-boot/next, thanks!
diff --git a/cmd/sf.c b/cmd/sf.c index f43a2e08b3..08e364e191 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> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[]) strncmp(argv[0], "write", 5) == 0) { int read; + if (CONFIG_IS_ENABLED(LMB)) { + if (lmb_read_check(addr, len)) { + printf("ERROR: trying to overwrite reserved memory...\n"); + return CMD_RET_FAILURE; + } + } + read = strncmp(argv[0], "read", 4) == 0; if (read) ret = spi_flash_read(flash, offset, len, buf); diff --git a/include/lmb.h b/include/lmb.h index fc2daaa7bf..aee2f9fcda 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -111,6 +111,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 */
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 V4: - Removed do_spi_read_lmb_check(). - Added the lmb_read_check() function in lmb.c, making it reusable for NAND, MMC, etc. - Addressed review comments. 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. 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... 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... Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000 SF: 16777216 bytes @ 0x0 Erased: OK device 0 offset 0x0, size 0x1000000 SF: 16777216 bytes @ 0x0 Written: OK device 0 offset 0x0, size 0x1000000 SF: 16777216 bytes @ 0x0 Read: OK Total of 16777216 byte(s) were the same cmd/sf.c | 8 ++++++++ include/lmb.h | 5 +++++ 2 files changed, 13 insertions(+)