From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 73AE9C4338F for ; Sun, 8 Aug 2021 14:00:27 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8A18A6101E for ; Sun, 8 Aug 2021 14:00:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8A18A6101E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C7E1982BF6; Sun, 8 Aug 2021 16:00:23 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="RUDi9dHM"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1B29782BF7; Sun, 8 Aug 2021 16:00:21 +0200 (CEST) Received: from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com [IPv6:2607:f8b0:4864:20::72f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 5048282051 for ; Sun, 8 Aug 2021 16:00:15 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qk1-x72f.google.com with SMTP id a19so15460773qkg.2 for ; Sun, 08 Aug 2021 07:00:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9wWrnXZaK6e+f8Al6ILVJuHeUQyldD1McfzvmJxCU7c=; b=RUDi9dHMudCmcFbFtyiwBN0cJbXKQ+SM1gjl7RRnSa0UlrwIPC/6IHrYW4WMGWhAp6 JxV/65QbjP5a/G3qyoM7rgVaySPnGFcHjYDmmVFkh4+Ca0OUNmd06QjRIrDRBDY2GoDB PqY/XYT6QVBw0f3RuE6hEH+OAuqUvdGWQIYtA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9wWrnXZaK6e+f8Al6ILVJuHeUQyldD1McfzvmJxCU7c=; b=dO1ewhJLxtH0KGHoiacY1a61QoKGbvrPy1hIquI0XY+FI7s7YL7ltoyHiPn1ar539O zD/rkI6gquB05okiI/M2XoAkFEwsJMMEoNwXOHOc8SOZWKOxzrzWYaSKsAl/SsR6vUNd Rk0BzcYthgdKE+/oYR2YEdDDlB+iBH+RcGr0nfUPH1XsdLFfwhEWuAA/6vtxeTrrigb+ ZkzhkyXdQAC2PuJc0IOxsPrUj4N0rKypqEeSL7licrCwg8s4FeVkekb5Gm9xUOLjW6cW XxXgcDjAiOtGxke5feWtSvFLpcMuO4PlaIsRmCTFKRqSYPobBMy6QUdtS09+H0X2vaYW sBYA== X-Gm-Message-State: AOAM531S/0ShLPO1r+LC8HMu/CmGPirGcuTW5FlLWvsjRy8DwXKYokjw m//WGN2OrEbn+RipZZIcDttjbw== X-Google-Smtp-Source: ABdhPJwW5CPnX8XXoyAKUdT9msrZDLsitesVLF4z1SkVio3eAnn3t1nbxwCHuDdy1RN/VEACCr3I+A== X-Received: by 2002:ae9:d619:: with SMTP id r25mr18859535qkk.177.1628431213884; Sun, 08 Aug 2021 07:00:13 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-399a-929e-f6a1-5ed4.res6.spectrum.com. [2603:6081:7b01:cbda:399a:929e:f6a1:5ed4]) by smtp.gmail.com with ESMTPSA id t10sm5902659qti.94.2021.08.08.07.00.12 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Sun, 08 Aug 2021 07:00:12 -0700 (PDT) Date: Sun, 8 Aug 2021 10:00:10 -0400 From: Tom Rini To: Marek Vasut Cc: Jan Kiszka , Wolfgang Denk , U-Boot Mailing List , Hai Pham , Simon Goldschmidt , Stephen Warren , Lokesh Vutla Subject: Re: [PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64" Message-ID: <20210808140010.GH858@bill-the-cat> References: <1971775f-28de-83d0-9459-a4e68c744a18@siemens.com> <20210802212759.GD9379@bill-the-cat> <20210803215144.GW9379@bill-the-cat> <015eed03-b945-8757-e994-17d17de45546@denx.de> <20210806164917.GB858@bill-the-cat> <4aff44db-6a83-bcce-a405-1662187983b2@denx.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="caC6Ojpb5aKGbDVu" Content-Disposition: inline In-Reply-To: <4aff44db-6a83-bcce-a405-1662187983b2@denx.de> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean --caC6Ojpb5aKGbDVu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > > > > >=20 > > > > > > From: Jan Kiszka > > > > > >=20 > > > > > > This reverts commit 2359fa7a87848626bcbd3399e92c657595880cd7. > > > > > >=20 > > > > > > While the goal is valid and there is surely unused memory in th= at area, > > > > > > we also have a lot of crucial things still located at the top-o= f-memory > > > > > > while running lmb_alloc_base. Such things are the page table (t= lb_addr), > > > > > > relocated U-Boot and the active stack. Possibly more. So this p= atch was > > > > > > premature, we will need relocations of those things first if we= want to > > > > > > use the range. > > > > > >=20 > > > > > > Fixes booting on the IOT2050, but likely also on other boards. = It got > > > > > > stuck on relocating the FDT - over the relocated U-Boot code. > > > > > >=20 > > > > > > Signed-off-by: Jan Kiszka > > > > > > --- > > > > > >=20 > > > > > > Practically, > > > > > >=20 > > > > > > void arch_lmb_reserve(struct lmb *lmb) > > > > > > { > > > > > > lmb_reserve(lmb, gd->relocaddr, gd->ram_top - gd->relocaddr); > > > > > > } > > > > > >=20 > > > > > > worked as well for me - but it left the stack in danger. > > > > >=20 > > > > > I want to cycle back up to this practically part. Marek, would c= hanging > > > > > 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 ove= rwrite > > > > > ourself and hang. > > >=20 > > > I think large part of the problem is the purpose of LMB is unclear at= best. > > >=20 > > > The arch_lmb_reserve() says this: > > >=20 > > > 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 */ > > >=20 > > > So basically reserve chunk of memory in the right place, which can be= safely > > > passed to the kernel. > >=20 > > No. This isn't the case. We reserve chunks of memory away from other > > usage by U-Boot. >=20 > 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. 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). > > > If that's not the case, and the LMB is used also to protect U-Boot fr= om > > > overwriting itself, then the above comment is totally misleading and = wrong > > > (and I have a feeling that is what you are trying to get on). > > >=20 > > > And if that's the case, then what we should reserve isn't > > > stack_bottom...ram_top, but really the work areas of U-Boot only (i.e= =2E a > > > more fine grained reservation might be needed). For starters, replaci= ng > > > ram_top with end of u-boot would do for my use case. > >=20 > > I addressed this I believe by going back and explaining what the code > > base did at the time this was added, and booting up the image on the > > platform it was tested on at the time. I think there's a case to be > > made that the comment is a bit misleading / unclear, when it was added. >=20 > I would say that is an understatement , but also see above. >=20 > > We can see when it was added CONFIG_SYS_BOOTMAPSZ being set to cover the > > start + 0x8000 or so of where ATAGS had traditionally been, and this > > function here covering where U-Boot was in memory, to avoid being > > overwritten by the device tree being relocated. > >=20 > > > > > An alternative that I'm not 100% sure I like would be adjusting > > > > > env_get_bootm_size() so that the case of bootm_size and bootm_low= are > > > > > unset, so we figure out a value based on gd->ram_top we also take= in to > > > > > account where U-Boot itself is atm and exclude that. > > >=20 > > > I think we should really clarify the purpose of the LMB and make sure= we > > > both understand it the same way. > > >=20 > > > > > It's possible that > > > > > if we had something like that at the start of the DT world, we wo= uldn't > > > > > have the code to disable fdt relocation, which really feels like = it's > > > > > largely been "things crash when we relocate stuff, disable reloca= tion" > > > > > and not so much "save a little boot time, we optimized things VERY > > > > > CAREFULLY". > > > >=20 > > > > Digging more. The comment about "Allocate space for command line a= nd > > > > board info" was introduced in 2d1916e48bd8 ("ARM: add flat device t= ree > > > > support"). I happen to have the platform this was tested on, a > > > > Beagleboard, around still and was able to boot that commit up and p= oke a > > > > bit. It's effectively covering all of the running U-Boot with an L= MB. > > > > You can move on to commit b485556be51d ("ARM: enable device tree for > > > > beagle"). That defines CONFIG_SYS_BOOTMAPSZ which is about start of > > > > DRAM + a bit, to reserve space there that would be questionable on > > > > ARM64. But all of that code area has been reworked since then. > > > >=20 > > > > So in sum, I think reverting 2359fa7a87848626bcbd3399e92c657595880c= d7 is > > > > right. U-Boot needs to be protected from being overwritten as it's > > > > still alive at that point and relocating things around. This has a= lways > > > > been true. This has always been related to device tree booting. > > > >=20 > > > > What rcar is trying to do needs to be better explained first so tha= t we > > > > can figure out the right place to make some sort of update. > > >=20 > > > rcar might not relocate u-boot all the way to the end of DRAM, becaus= e there > > > might be some reserved memory at the end of DRAM used by one of the o= ther > > > CPUs in the SoC, that's all. > >=20 > > OK, so then there isn't a problem reverting this commit for rcar? >=20 > 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=20 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. --=20 Tom --caC6Ojpb5aKGbDVu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmEP42MACgkQFHw5/5Y0 tyzpHAv/VrOaiPwThWJUssDIitjyeAYHQWL137FaBWIXqtSHMWa5qpypRZZ98jFs aLyjn+UHqvcyswcNBJmTpq/ZlVd+E+UMt1U652ePtr24vefAYdIGVl2sI+oblz6I bwuX8vxv7j2lL67mBYSA9EdXqvaKXuKUkuu7DHQTyGXUJqNopJYOTEr20GU9N0r2 DtDvZl6maSZ3f9v1o713GvO0GEg9mAHiUyPzVc2v5q3hSzRGp9LIhoOiv3pFXCsT NdpyiSgzSwwh2ocrnRUSpl2yXl56XQ8MMYDWvDBEK8M/D5+606JvazZCr+fGfGak b5KxZ9W6z3oozitd6PLV3vgWGGb5dKpljOEZFJ1m4j/m3KW1AbtvMiW6qQG1K7Lg yyeYi+8pohcq55qx9WwDHE8wFBRACGYMgCX4l1tNcflz/q6s8OxKQUaApkN5JeqZ kW7bdGyT4jtruN30EShSL7zEOmZJ3ivG8YIRoKlCEQDfzkT1e0ReM/qg0XYJIYch Trl7dTop =pfyY -----END PGP SIGNATURE----- --caC6Ojpb5aKGbDVu--