Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5765 bytes --]

On Sun, Aug 08, 2021 at 04:28:14PM +0200, Marek Vasut wrote:
> On 8/8/21 4:00 PM, Tom Rini wrote:
> > On Sun, Aug 08, 2021 at 03:45:30PM +0200, Marek Vasut wrote:
> > > On 8/6/21 6:49 PM, Tom Rini wrote:
> > > > On Fri, Aug 06, 2021 at 12:22:43AM +0200, Marek Vasut wrote:
> > > > > On 8/3/21 11:51 PM, Tom Rini wrote:
> > > > > > On Mon, Aug 02, 2021 at 05:27:59PM -0400, Tom Rini wrote:
> > > > > > > On Thu, Jul 29, 2021 at 09:22:02AM +0200, Jan Kiszka wrote:
> > > > > > > 
> > > > > > > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > > > > 
> > > > > > > > This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7.
> > > > > > > > 
> > > > > > > > While the goal is valid and there is surely unused memory in that area,
> > > > > > > > we also have a lot of crucial things still located at the top-of-memory
> > > > > > > > while running lmb_alloc_base. Such things are the page table (tlb_addr),
> > > > > > > > relocated U-Boot and the active stack. Possibly more. So this patch was
> > > > > > > > premature, we will need relocations of those things first if we want to
> > > > > > > > use the range.
> > > > > > > > 
> > > > > > > > Fixes booting on the IOT2050, but likely also on other boards. It got
> > > > > > > > stuck on relocating the FDT - over the relocated U-Boot code.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Practically,
> > > > > > > > 
> > > > > > > > void arch_lmb_reserve(struct lmb *lmb)
> > > > > > > > {
> > > > > > > > 	lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr);
> > > > > > > > }
> > > > > > > > 
> > > > > > > > worked as well for me - but it left the stack in danger.
> > > > > > > 
> > > > > > > I want to cycle back up to this practically part.  Marek, would changing
> > > > > > > the arch_lmb_reserve (or possibly even making this a more global thing /
> > > > > > > option) still let the area you want exposed on rcar3 (I assume) be
> > > > > > > exposed ?  Or would it be covered again?  Part of the problem,
> > > > > > > practically speaking, is that we need to ensure that as part of
> > > > > > > (attempting and likely succeeding in) booting the OS we don't overwrite
> > > > > > > ourself and hang.
> > > > > 
> > > > > I think large part of the problem is the purpose of LMB is unclear at best.
> > > > > 
> > > > > The arch_lmb_reserve() says this:
> > > > > 
> > > > >    54 void arch_lmb_reserve(struct lmb *lmb)
> > > > > [...]
> > > > >    59 /*
> > > > >    60  * Booting a (Linux) kernel image
> > > > >    61  *
> > > > >    62  * Allocate space for command line and board info - the
> > > > >    63  * address should be as high as possible within the reach of
> > > > >    64  * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused
> > > > >    65  * memory, which means far enough below the current stack
> > > > >    66  * pointer.
> > > > >    67  */
> > > > > 
> > > > > So basically reserve chunk of memory in the right place, which can be safely
> > > > > passed to the kernel.
> > > > 
> > > > No.  This isn't the case.  We reserve chunks of memory away from other
> > > > usage by U-Boot.
> > > 
> > > Then I have to wonder, why is this not called in board_init_f or
> > > board_init_r , but only after bootm or similar boot command was called ?
> > 
> > I also wonder a little bit.  That it does not is why we have the LMB
> > checks under fs/ and net/ and doing this sooner would possibly make
> > dealing with those CVEs either easier or would also address some other
> > classes of them that may exist.
> 
> So, why not move it into the relocation code then ?

A further potential clean-up, yes.  I don't think that was discussed
around CVE-2018-18439 / CVE-2018-18440.

> > I expect it was not simply because up
> > until rather recently we didn't have any checks for "don't overwrite
> > specific areas of memory" other than right before firing off the OS (and
> > modify whatever memory you want to modify is a feature not a bug).
> 
> The LMB has been around since forever though ?

Yes, LMB has been around since the PowerPC device tree days I suspect (I
didn't dig that far back), but only used outside of the "don't overwrite
the running U-Boot while we relocate device tree / initrd before booting
OS" since 2018 or so.

> [...]
> 
> > > > OK, so then there isn't a problem reverting this commit for rcar?
> > > 
> > > The revert will break the use case where the other CPUs are using memory
> > > above U-Boot, but have a look at the following branch, it should permit me
> > > to parametrize the arch_lmb_reserve() better and reserve the right memory
> > > areas per architecture/mach/board, and even clean the arch_lmb_reserve up
> > > further:
> > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/tree/lmb-v1
> > > So yes, pick the revert and I'll submit the four patches for likely next
> > > release.
> > 
> > Thanks for explaining, I'll pick up the revert patch then.
> > 
> > For your LMB tree, I like the initial approach but looking at
> > 528915c71762 ("imx: Fix potential lmb memory overwritten by stack") I
> > think that shows the general "4K is enough for stack we hope" is wrong,
> > and we should do 16K instead for everyone as the default.  But we can
> > discuss that more too once you post the whole series which again, I
> > think is the right direction.
> 
> The IMX thing is odd indeed and raises a bigger question -- what is the
> "right" amount of stack to reserve ?

It's a good question, yes.  And some more details about what exactly the
NXP folks were doing to hit that would also be nice.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]