On Mon, 16 Sept 2024 at 11:10, Vaishnav Achath <vaishna...@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 <n...@ti.com>
> Signed-off-by: Vaishnav Achath <vaishna...@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@addr=0x82000000 75416 bytes: Success!
> 75416 bytes read in 12 ms (6 MiB/s)
> Load Remote Processor 3 with data@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
>
