[PATCH v3] cmd: sf: prevent overwriting the reserved memory

[Sughosh Ganu]

On Wed, 11 Sept 2024 at 16:53, Michal Simek <michal.simek at amd.com> wrote:
>
>
>
> On 9/11/24 13:20, Sughosh Ganu wrote:
> > On Tue, 10 Sept 2024 at 21:14, Prasad Kummari <prasad.kummari at 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 at 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