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

[Simon Glass]

Hi Heinrich,

On Thu, 13 Jun 2024 at 11:32, 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.

Hmm you mentioned that before but I'm still not quite understanding.
Do you mean that the EFI app returns back to U-Boot, leaving the
driver active? If so, how does U-Boot use the driver? I'm just not
familiar with how such a construct could work in a single-threaded
U-Boot.

>
> 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.

The implementation of pool memory rounds up to pages and uses that. It
might be more efficient to use malloc()/free() instead, given that we
mark the malloc() space as reserved when we call the EFI app.

>
> We should not make any assumptions that conflict with the UEFI
> specification.

Indeed.

>
> 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.
>
> 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
>