Hi Sughosh,
> -----Original Message-----
> From: Sughosh Ganu <sughosh.g...@linaro.org>
> Sent: Wednesday, September 11, 2024 5:26 PM
> To: Simek, Michal <michal.si...@amd.com>
> Cc: Kummari, Prasad <prasad.kumm...@amd.com>; u-boot@lists.denx.de;
> git (AMD-Xilinx) <g...@amd.com>; Abbarapu, Venkatesh
> <venkatesh.abbar...@amd.com>; Begari, Padmarao
> <padmarao.beg...@amd.com>; g...@xilinx.com;
> ja...@amarulasolutions.com; n-fran...@ti.com; d-g...@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.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.
[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
