[RFC PATCH 04/31] lmb: remove local instances of the lmb structure variable

[Simon Glass]

Hi Sughosh,

On Thu, 13 Jun 2024 at 12:17, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Thu, 13 Jun 2024 at 22:57, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 13.06.24 18:59, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 13 Jun 2024 at 09:42, Tom Rini <trini at konsulko.com> wrote:
> > >>
> > >> On Thu, Jun 13, 2024 at 09:22:15AM -0600, Simon Glass wrote:
> > >>> Hi Tom,
> > >>>
> > >>> On Wed, 12 Jun 2024 at 15:40, Tom Rini <trini at konsulko.com> wrote:
> > >>>>
> > >>>> On Wed, Jun 12, 2024 at 02:24:25PM -0600, Simon Glass wrote:
> > >>>>> Hi Tom,
> > >>>>>
> > >>>>> On Wed, 12 Jun 2024 at 11:22, Tom Rini <trini at konsulko.com> wrote:
> > >>>>>>
> > >>>>>> On Tue, Jun 11, 2024 at 08:41:39PM -0600, Simon Glass wrote:
> > >>>>>>
> > >>>>>> [snip]
> > >>>>>>> Also IMO there is only really one LMB list today. We create it at the
> > >>>>>>> start of bootm and then it is done when we boot. The file-loading
> > >>>>>>> stuff is what makes all this confusing...and with bootstd that is
> > >>>>>>> under control as well.
> > >>>>>>>
> > >>>>>>> At lot of this effort seems to be about dealing with random scripts
> > >>>>>>> which load things. We want to make sure we complain if something
> > >>>>>>> overlaps. But we should be making the bootstd case work nicely and
> > >>>>>>> doing things within that framework. Also EFI sort-of has its own
> > >>>>>>> thing, which it is very-much in control of.
> > >>>>>>>
> > >>>>>>> Overall I think this is a bit more subtle that just combining allocators.
> > >>>>>>
> > >>>>>> I think this gets to the main misunderstanding. The problem isn't
> > >>>>>> handling bootstd, or EFI boot, or even assorted scripts. Those are all
> > >>>>>> cases where things are otherwise (sufficiently) well-defined. The
> > >>>>>> problem is "security" and that a "carefully crafted payload" could do
> > >>>>>> something malicious. That's why we have to do all of this stuff sooner
> > >>>>>> rather than later in our boot process.
> > >>>>>
> > >>>>> That's the first I have heard of this, actually, but a bit more detail
> > >>>>> would help. How does the payload get loaded? I'm just not sure about
> > >>>>> the overall goals. It seems that everyone else is already familiar -
> > >>>>> can someone please take the time to point me to the details?
> > >>>>
> > >>>> Well, the short version I believe of the first CVE we got (and so
> > >>>> started abusing LMB) was along the lines of "load an image near where
> > >>>> the U-Boot stack is, smash things for fun and exploits".
> > >>>
> > >>> OK. I am surprised that LMB does not catch that. It is supposed to add
> > >>> the stack and various other things right at the start before loading
> > >>> any file. So even if it clears the LMB each time, it should not be
> > >>> able to do that. Having said this, the code may be buggy as I don't
> > >>> think we have tests for U-Boot's overall functional behaviour in these
> > >>> situations.
> > >>
> > >> Right, LMB does catch the example I gave (because we made all of the
> > >> load from storage/network functions init an lmb and we always make sure
> > >> a new lmb gets U-Boot stack/etc). The next thing we didn't catch was
> > >> "what if EFI does the loading?" and we've kludged around that, and in
> > >> turn had some of the thorny questions. Some of that is what I think
> > >> you're asking about in this part of the thread, to which the answer is
> > >> "EFI spec says you need to place X in memory", so we just need to
> > >> reserve it when it's asked for, so that something else can't come along
> > >> and smash it maliciously.
> > >
> > > OK I see. Of course it isn't just EFI that has this issue. I believe
> > > the answer (for small blocks) is to use malloc(), which I think we do
> > > with a few exceptions which Ilias pointed out. For things like the TPM
> > > log and ACPI tables we should probably use a bloblist, as we do on
> > > x86. For large things (like loading a kernel) we should use LMB. I've
> > > been thinking about how best to tie this to boot, as opposed to random
> > > allocations in U-Boot itself, which would lead to fragmentation and
> > > strange behaviour. I think bootstd is a great place to have a
> > > persistent LMB. It can be attached to bootstd_priv.
> > >
> > > My hope is that EFI is just another boot method, where
> > > already-allocating things are presented to the OS. Apart from the
> > > Ilias exceptions, I believe this is how it works today.
> > >
> > > Where I think this heads in the wrong direction is using
> > > EFI-allocation functions before we are booting an EFI image. EFI has
> > > no concept of what is 'in empty space' so it leads to the lmb
> > > conflict, the subject of this discussion.
> >
> > EFI binaries can return to the command line interface.
> > EFI binaries may be drivers that stay resident and run in the background
> > after returning to the command line interface. They might for instance
> > provide block devices.
> >
> > Device-paths must be created from EFI pool memory as they may be freed
> > via FreePool() according to the EFI specification. And these we create
> > whenever a block-device is probed.
> >
> > We should not make any assumptions that conflict with the UEFI
> > specification.
> >
> > In our initial discussion with Ilias one idea was to merge LMB and EFI
> > memory management. This merged system would have to consider the
> > requirements of the UEFI specifications like a finer grained memory type
> > system and page boundaries.
>
> Not sure about merging LMB and EFI memory management, but I am looking
> at using the LMB API's for EFI allocations for the next version. The
> LMB API's as they stand today, align pretty neatly with the
> requirements of the efi_allocate_pages() function, which really is the
> EFI memory allocation function. I think it should be possible to use
> the LMB API's(with a different flag passed to differentiate between
> LMB and EFI reservations) from the EFI memory module. I think things
> like maintaining the EFI memory map should still reside in the EFI
> memory module.

Yes, that makes a lot of sense to me.

Regards,
Simon

>
> -sughosh
>
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > This is all quite subtle and probably worthy of a VC discussion.
> > >
> > >>
> > >> But that also raised the more general problem, and why we need a
> > >> persistent reservation list, of allowing boards/SoCs to say they want to
> > >> reserve a block of memory for whatever, and have that obeyed, for real.
> > >> For example, the mach-apple logic of "just pick some memory locations to
> > >> use for kernel/dtb/initrd" isn't really as safe as it should be since
> > >> those reservations aren't really seen anywhere once the function
> > >> returns, it's just setting some environment variables.
> > >
> > > Yes, that part of it I understand. Somehow I either didn't see or
> > > forgot that board_late_init() code. With the script-based boot it
> > > makes some sort of sense, but with bootstd we should have allocation
> > > of addresses dealt with there. I have held off on retiring
> > > kernel_addr_r etc. as the scripts are still in use. But perhaps it
> > > would be a good time to convert bootstd to use lmb instead?
> > >
> > > Regards,
> > > Simon
> >