[PATCH v4 05/27] lmb: make LMB memory map persistent and global

[Sughosh Ganu]

On Mon, 16 Sept 2024 at 14:09, Vaishnav Achath <vaishnav.a at 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 at 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 at linaro.org>
> >>> Signed-off-by: Simon Glass <sjg at 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+tFUkA@mail.gmail.com/
[2] - https://lore.kernel.org/u-boot/20231229154307.GS2652760@bill-the-cat/