On Mon, 16 Sept 2024 at 14:09, Vaishnav Achath <vaishna...@ti.com> wrote: > > Hi Sughosh, > > On 16/09/24 11:59, Sughosh Ganu wrote: > > On Mon, 16 Sept 2024 at 11:22, Vaishnav Achath <vaishna...@ti.com> wrote: > >> > >> Hi Sughosh, > >> > >> On 26/08/24 17:29, Sughosh Ganu wrote: > >>> The current LMB API's for allocating and reserving memory use a > >>> per-caller based memory view. Memory allocated by a caller can then be > >>> overwritten by another caller. Make these allocations and reservations > >>> persistent using the alloced list data structure. > >>> > >>> Two alloced lists are declared -- one for the available(free) memory, > >>> and one for the used memory. Once full, the list can then be extended > >>> at runtime. > >>> > >>> Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org> > >>> Signed-off-by: Simon Glass <s...@chromium.org> > >>> [sjg: Optimise the logic to add a region in lmb_add_region_flags()] > >>> [sjg: Use a stack to store pointer of lmb struct when running lmb tests] > >>> --- > >>> Changes since V3: > >>> * Fix checkpatch warnings of spaces between function name and > >>> open parantheses. > >>> * s/uint64_t/u64 as suggested by checkpatch. > >>> * Remove unneccessary parantheses in lmb.c as suggested by checkpatch. > >>> * Fix alignment in test/cmd/bdinfo.c as suggested by checkpatch. > >>> > >>> arch/arc/lib/cache.c | 4 +- > >>> arch/arm/lib/stack.c | 4 +- > >>> arch/arm/mach-apple/board.c | 17 +- > >>> arch/arm/mach-snapdragon/board.c | 17 +- > >>> arch/arm/mach-stm32mp/dram_init.c | 8 +- > >>> arch/arm/mach-stm32mp/stm32mp1/cpu.c | 6 +- > >>> arch/m68k/lib/bootm.c | 7 +- > >>> arch/microblaze/lib/bootm.c | 4 +- > >>> arch/mips/lib/bootm.c | 11 +- > >>> arch/nios2/lib/bootm.c | 4 +- > >>> arch/powerpc/cpu/mpc85xx/mp.c | 4 +- > >>> arch/powerpc/include/asm/mp.h | 4 +- > >>> arch/powerpc/lib/bootm.c | 14 +- > >>> arch/riscv/lib/bootm.c | 4 +- > >>> arch/sh/lib/bootm.c | 4 +- > >>> arch/x86/lib/bootm.c | 4 +- > >>> arch/xtensa/lib/bootm.c | 4 +- > >>> board/xilinx/common/board.c | 8 +- > >>> boot/bootm.c | 26 +- > >>> boot/bootm_os.c | 5 +- > >>> boot/image-board.c | 34 +- > >>> boot/image-fdt.c | 35 +- > >>> cmd/bdinfo.c | 6 +- > >>> cmd/booti.c | 2 +- > >>> cmd/bootz.c | 2 +- > >>> cmd/elf.c | 2 +- > >>> cmd/load.c | 7 +- > >>> drivers/iommu/apple_dart.c | 8 +- > >>> drivers/iommu/sandbox_iommu.c | 16 +- > >>> fs/fs.c | 7 +- > >> > >> In fs the reserved region is not being freed after read, while > >> other loaders free them (cmd/load.c), this patch uncovers the issue > >> since now other loaders cannot use the same memory location for load. > > > > The idea with the LMB memory is that it is not really an allocation, > > but setting aside memory for use. Now there was a discussion earlier > > on the mailing list if this is actually an allocation or not, but this > > is what the functions have been called from it's early days. But the > > way the code is designed now, even with the global and persistent > > memory map, we have the LMB_NONE flag which is used to allow for the > > same memory region to be re-allocated/re-reserved. > > > > Understood, thanks for explaining, since the LMB memory map is now > global, anytime you run these commands a region is marked reserved and > it shows up in lmb_dump_all(), while it is not necessary to free them, > there are callers which may error out without reclaiming the region, > as long as these callers exist systems can break due to this change so a > cleanup is needed. > > >> For example now someone cannot do: > >> > >> mmc load .. $loadaddr ... > >> <do something with above contents> > >> tftp $loadaddr .. > > > > The issue above is what I mentioned to Prasad in one of my earlier > > replies to the patch that he had sent [1]. What can be done is to > > unify the manner in which callers ask for LMB memory -- that would > > mean changing the behaviour of the tftp code to use the logic used in > > the fs_read_lmb_check() function. I believe this method of loading to > > an address is more beneficial as it allows memory re-use. > > > > As per my understanding, these checks were added in tftp/wget/fs loaders > due to CVE-2018-18440, the intent is to only ensure that there is no > overwrite to existing reserved regions, there is no need to reserve the > current load buffer region, what fs_read_lmb_check() does not seem to be > the right approach and scaling that does not seem right even though it > fixes the problem being discussed.
The approach taken by fs_read_lmb_check() is inline with the current expectation of how a memory region for loading an image is supposed to behave. There was a long discussion on the mailing list [1] about whether the LMB memory allocations are to be perceived as actual allocations (one where an alloc is supposed to have a corresponding free), and it was pointed out [2] that historically, that is not how a LMB memory behaves. Hence this needs to be looked at a little differently than the usual construct of "an allocation needs to have a corresponding free". -sughosh [1] - https://lore.kernel.org/u-boot/caflsztict2yzsm+kajryqzv3bt1_w2xyjacexx-lhnz6+tf...@mail.gmail.com/ [2] - https://lore.kernel.org/u-boot/20231229154307.GS2652760@bill-the-cat/
