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

[Marek Vasut]

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 at 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 at 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 ?

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

[...]

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