[PATCH] fs: Fix reserved memory alloaction in fs_read()

[Sughosh Ganu]

On Mon, 16 Sept 2024 at 11:10, Vaishnav Achath <vaishnav.a at ti.com> wrote:
>
> The reserved memory region in fs_read() is not being
> freed after read, free that. The issue was uncovered by
> commit ed17a33fed29 ("lmb: make LMB memory map persistent and global")
> as now the region used to load from fs cannot be reused by other
> loaders like tftp, wget which use lmb_get_free_size() instead
> of lmb_alloc_addr(), other loaders which uses lmb_reserve() is
> appropriately freeing the buffer after usage (Eg. Serial loader).
> Also while at it move #if CONFIG_IS_ENABLED(LMB) to if (IS_ENABLED()).
>
> Fixes aa3c609e2be5 ("fs: prevent overwriting reserved memory")
>
> Reported-by: Nishanth Menon <nm at ti.com>
> Signed-off-by: Vaishnav Achath <vaishnav.a at ti.com>
> ---

Just for the record, this patch does not need to be applied. The
load-address based LMB checks in the tftp and wget functions have been
tweaked so that this issue will no longer be relevant -- those patches
have been tested by Vaishnav to confirm the same. Thanks.

-sughosh

>
> We started seeing boot failures since we have platforms that perform
> mmc load for firmwares and subsequent tftp load for kernel to same
> loadaddr, the reservation need not be permanent and is only to make
> sure that the read is not overwriting reserved regions, there is no
> permanent reservation/protection needed for each load.
>
> In failing case (J7200 EVM):
>
> _snip_
> run boot_rprocs
> 75416 bytes read in 11 ms (6.5 MiB/s)
> Load Remote Processor 2 with data at addr=0x82000000 75416 bytes: Success!
> 75416 bytes read in 12 ms (6 MiB/s)
> Load Remote Processor 3 with data at addr=0x82000000 75416 bytes: Success!
> _snip_
> lmb_dump_all:
>  memory.count = 0x1
>  memory[0]    [0x80000000-0xffffffff], 0x80000000 bytes flags: none
>  reserved.count = 0x3
>  reserved[0]    [0x82000000-0x82012697], 0x00012698 bytes flags: none (this is from previous mmc load, size == 12698h = 75416)
>  reserved[1]    [0x9e800000-0xa47fffff], 0x06000000 bytes flags: no-map
>  reserved[2]    [0xfce8fef0-0xffffffff], 0x03170110 bytes flags: no-overwrite
>
> With fix:
> lmb_dump_all:
>  memory.count = 0x1
>  memory[0]    [0x80000000-0xffffffff], 0x80000000 bytes flags: none
>  reserved.count = 0x2
>  reserved[1]    [0x9e800000-0xa47fffff], 0x06000000 bytes flags: no-map
>  reserved[2]    [0xfce8fef0-0xffffffff], 0x03170110 bytes flags: no-overwrite
>
>  fs/fs.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 4bc28d1dff..e00c2056a5 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -552,7 +552,7 @@ static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset,
>         lmb_dump_all();
>
>         if (lmb_alloc_addr(addr, read_len) == addr)
> -               return 0;
> +               return read_len;
>
>         log_err("** Reading file would overwrite reserved memory **\n");
>         return -ENOSPC;
> @@ -564,15 +564,13 @@ static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>  {
>         struct fstype_info *info = fs_get_info(fs_type);
>         void *buf;
> -       int ret;
> +       int ret, alloc_size;
>
> -#if CONFIG_IS_ENABLED(LMB)
> -       if (do_lmb_check) {
> -               ret = fs_read_lmb_check(filename, addr, offset, len, info);
> -               if (ret)
> -                       return ret;
> +       if (IS_ENABLED(CONFIG_LMB) && do_lmb_check) {
> +               alloc_size = fs_read_lmb_check(filename, addr, offset, len, info);
> +               if (alloc_size < 0)
> +                       return alloc_size;
>         }
> -#endif
>
>         /*
>          * We don't actually know how many bytes are being read, since len==0
> @@ -587,6 +585,9 @@ static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>                 log_debug("** %s shorter than offset + len **\n", filename);
>         fs_close();
>
> +       if (IS_ENABLED(CONFIG_LMB) && do_lmb_check)
> +               lmb_free(addr, alloc_size);
> +
>         return ret;
>  }
>
> --
> 2.34.1
>