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.kis...@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.kis...@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
signature.asc
Description: PGP signature