[PATCH v4 05/27] lmb: make LMB memory map persistent and global
Sughosh Ganu
sughosh.ganu at linaro.org
Mon Sep 16 20:11:00 CEST 2024
On Mon, 16 Sept 2024 at 22:19, Vaishnav Achath <vaishnav.a at 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 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".
> >
>
> 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+tFUkA@mail.gmail.com/
> > [2] - https://lore.kernel.org/u-boot/20231229154307.GS2652760@bill-the-cat/
> >
> >
More information about the U-Boot
mailing list