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
