On Mon, 16 Sept 2024 at 22:19, Vaishnav Achath vaishnav.a@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 vaishnav.a@ti.com wrote:
Hi Sughosh,
On 16/09/24 11:59, Sughosh Ganu wrote:
On Mon, 16 Sept 2024 at 11:22, Vaishnav Achath vaishnav.a@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.ganu@linaro.org Signed-off-by: Simon Glass sjg@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... [2] - https://lore.kernel.org/u-boot/20231229154307.GS2652760@bill-the-cat/