On Mon, 16 Sept 2024 at 22:19, Vaishnav Achath <vaishna...@ti.com> wrote: > > Hi Sughosh, > > Requesting clarification for one more point, > > On 16/09/24 14:46, Sughosh Ganu wrote: > > 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". > > > > Generally for the reserved memory allocations coming from device tree > the no-overwrite flag is not present, but the expectation is that these > are important regions that are expected to be reserved (Example ATF, > OPTEE, Remote core FW regions in DDR), but now each consumer is able to > rewrite/re-allocate the region again by these loaders (previously most > could not rewrite since they used lmb_get_free_size() which only > performs a check), isn't this a valid case where all the consumers > should not have privilege to overwrite these allocations? Or should all > regions from DT be marked as no-overwrite?
Yes, these regions need to be marked as no-overwrite to ensure that we get an error if an address in that region is being used as a load-address. Do you want to send a patch ? -sughosh > > > Thanks and Regards, > Vaishnav > > > -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/ > > > >
