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=-7.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 75324C433F5 for ; Sat, 4 Sep 2021 14:10:34 +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 8F1C460F12 for ; Sat, 4 Sep 2021 14:10:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8F1C460F12 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 2AE9C83259; Sat, 4 Sep 2021 16:10:31 +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="HNLPvyQq"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E980C83265; Sat, 4 Sep 2021 16:10:28 +0200 (CEST) Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) (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 A850882F10 for ; Sat, 4 Sep 2021 16:10:24 +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-qt1-x831.google.com with SMTP id g11so1698176qtk.5 for ; Sat, 04 Sep 2021 07:10:24 -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=EW69Jiz5wvGxC/B2NZHFGdscNwbtoVH7LNSAv9r+Fn0=; b=HNLPvyQqehhzRWBKsHtrzFeYNi/ZVStAmnmUZ333c09sBwW4A1hvCrpCo1fPE89OmY nUi1Kh52eLabvZwWSsOdLWKmdCehAn03LQc+G+d1RNxpXdfOuc8+4cEdWBvZxQ+aT993 IUiG8ugqLyfxH5/veoU2SbTpGsC2sG51txZus= 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=EW69Jiz5wvGxC/B2NZHFGdscNwbtoVH7LNSAv9r+Fn0=; b=cJZLWD/XmVHhCKTJiU//WC/dyU/FOmmdvh5aNcC3MMyNfC14tC8OgGGlEvZpjWwb+V drbprxrmh82nBbJv1IUYjfKvzWS9PdjjZ7zUP2FEegLJaHgGP/tGNSltqYMuCVpptsnM tte1dVve3lU8UocRWFuivVRbpiGz3hdDhgwcb5sjPQFd7uEHp8asTC39g11Vah8ExCll QBF1u7Wm7O9xJTKQ1ZTFCQTlcVa5UE2+QyK6y8kmyh8ta7VLuMWybWII6t97UYLUS90O gP3VHTSWPsmJnbIkONp2uQG8wvPgnByuuKIlrBW+7zfKa1QvlWQgZt1ReoGQwzKRG0L6 I8ww== X-Gm-Message-State: AOAM5336JvTJ61hyWjrO2aBJuz1/gfD4ca5YTzEnrv4cPMi3SP3PLlMN BdEZvIDtUaj3mSIzGVZZsb7HUA== X-Google-Smtp-Source: ABdhPJyt4JTzqAkgTgSHGNlIEDAviZMrsrDsNXKjl/bNwymPzWiFi8iP+kKy32owFGmLIeRCKR++Rw== X-Received: by 2002:ac8:5753:: with SMTP id 19mr3646823qtx.202.1630764623393; Sat, 04 Sep 2021 07:10:23 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-eda5-0c2a-e600-b83e.res6.spectrum.com. [2603:6081:7b01:cbda:eda5:c2a:e600:b83e]) by smtp.gmail.com with ESMTPSA id e22sm1622826qte.57.2021.09.04.07.10.21 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Sat, 04 Sep 2021 07:10:21 -0700 (PDT) Date: Sat, 4 Sep 2021 10:10:19 -0400 From: Tom Rini To: Marek Vasut Cc: u-boot@lists.denx.de, Simon Glass , Simon Goldschmidt Subject: Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined Message-ID: <20210904141019.GC12964@bill-the-cat> References: <20210829221008.GK858@bill-the-cat> <0f735fc1-c331-ee13-4bbf-5be71d45fecd@denx.de> <20210829222356.GL858@bill-the-cat> <2cffb09e-23aa-afce-962e-1eb9727457db@denx.de> <20210829225126.GM858@bill-the-cat> <20210829231145.GN858@bill-the-cat> <95c965a3-2da8-1212-fb54-0d5628d7743b@denx.de> <20210830120102.GO858@bill-the-cat> <678a77d9-356a-e924-da8e-102fba938ee3@denx.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="oTHb8nViIGeoXxdp" Content-Disposition: inline In-Reply-To: <678a77d9-356a-e924-da8e-102fba938ee3@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 --oTHb8nViIGeoXxdp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Sep 04, 2021 at 04:03:25PM +0200, Marek Vasut wrote: > On 8/30/21 2:01 PM, Tom Rini wrote: >=20 > [...] >=20 > > > > > > > > > > > > > > > > We shouldn't need this at all. LMB and LMB= _USE_MAX_REGIONS are both in > > > > > > > > > > > > > > > > Kconfig and have the dependencies expressed= that way. > > > > > > > > > > > > > > >=20 > > > > > > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG= _LMB_RESERVED_REGIONS may be > > > > > > > > > > > > > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_M= AX_REGIONS . They are four > > > > > > > > > > > > > > > different symbols. > > > > > > > > > > > > > >=20 > > > > > > > > > > > > > > I'm still not seeing it, sorry. Is there some = case where we're trying > > > > > > > > > > > > > > to access a struct lmb without CONFIG_LMB enabl= ed? > > > > > > > > > > > > > >=20 > > > > > > > > > > > > >=20 > > > > > > > > > > > > > See build failure > > > > > > > > > > > > > https://source.denx.de/u-boot/custodians/u-boot-s= h/-/jobs/315331 > > > > > > > > > > > >=20 > > > > > > > > > > > > Ah, progress. Drop from since we= already have a > > > > > > > > > > > > forward declaration of struct lmb? But it's not fa= iling without this > > > > > > > > > > > > series too, so what's changing? > > > > > > > > > > >=20 > > > > > > > > > > > See 01/14 in this series. > > > > > > > > > >=20 > > > > > > > > > > Ah, so drop 1/14 then. > > > > > > > > >=20 > > > > > > > > > Why ? That patch is correct. > > > > > > > >=20 > > > > > > > > It's not quite right, 1/14 and then 2/14 are papering over = the fact that > > > > > > > > lmb.h, and it's including headers / files, need to be clean= ed up so that > > > > > > > > we don't need to have redundant tests in the header. > > > > > > >=20 > > > > > > > 1/14 disables LMB and CMD_BDI for tools build, we do not need= those, so 1/14 > > > > > > > is correct. > > > > > >=20 > > > > > > We don't need to build u-boot at all for tools-only, only the t= ools-only > > > > > > build target. It's just annoying to exclude the tools-only_def= config from > > > > > > "sandbox" in CI. > > > > >=20 > > > > > So, what exactly is the problem with that 01/14 ? Please elaborat= e, I > > > > > believe the patch is correct. > > > >=20 > > > > You disable LMB in a target that's only building "all" in CI because > > > > wasn't ever worth adding ",sandbox" to the all other arches job unt= il > > > > perhaps now. > > > >=20 > > > > Disabling LMB in tools-only_defconfig then exposes that can= only > > > > be included safely when CONFIG_LMB is set. > > > >=20 > > > > Adding / extending an #if test in code for something that's already > > > > checked for in Kconfig is bad. We spent so much time already remov= ing > > > > and shrinking #if tests in the code. > > >=20 > > > So, the patch is correct, the headers need further clean up. > >=20 > > No, it's not. The first patch is wrong because disabling CONFIG_LMB is > > invalid. >=20 > Please explain why the patch disabling LMB support for tools-only build is > invalid. I disagree with this statement, LMB support in tools-only build = is > useless, because LMB protects parts of running U-Boot from being > overwritten. >=20 > > The second patch is conceptually wrong because if we're > > enforcing a check in C for a dependency that's enforced in Kconfig, we > > have another problem to investigate. Which I did, LMB is non-optional. >=20 > Please explain why is LMB non-optional ? I disagree. LMB for tools-only > build is useless, hence it should not be enabled. >=20 > > > > > > > What kind of cleanup of lmb.h do you have in mind ? > > > > > >=20 > > > > > > Remove it from include/image.h and fix any fall-out from that o= f files > > > > > > that got indirectly when they needed it directly instea= d. > > > > >=20 > > > > > Uh ... that is likely for a separate series, and a big one. > > > >=20 > > > > Honestly, checking again, I'm not sure LMB=3Dn is valid, ever. > > >=20 > > > Why wouldn't it be ? For tools, LMB=3Dn is perfectly valid. > >=20 > > Because it's never valid to disable LMB, LMB is what protects the > > running U-Boot. >=20 > We are talking about tools-only build here, not running U-Boot. >=20 > > It's nonsense to build u-boot on tools-only_defconfig but we don't have > > a way currently to remove "u-boot" from the all target. Maybe once a > > few more of the hard/tricky CONFIG symbols get migrated to Kconfig we > > can then set tools-only_defconfig to NOT build u-boot at all. > >=20 > > > > That's how > > > > we keep our running U-Boot from being trivially overwritten and a h= uge > > > > number of security issues from being re-opened. > > >=20 > > > Tools are not running U-Boot. > > >=20 > > > > At this point, I think you should rework things to stop making > > > > CONFIG_LMB be optional, it should be a def_bool y. > > >=20 > > > I disagree, see above. > >=20 > > The only reason "tools-only_defconfig" builds a useless u-boot binary > > today is in CI where it would be more work than it's worth to make CI > > exclude that from the build list. But if you want to just do that > > instead, I'll also accept adding -x tools-only to the azure/gitlab jobs > > that build all other architectures, as tools-only is tested in its own > > build job, for it's only valid build target. >=20 > The tools-only build is also used elsewhere, to build just that, tools. I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y. --=20 Tom --oTHb8nViIGeoXxdp Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmEzfkgACgkQFHw5/5Y0 tywWRwwAmnM+/jjt2zGp8nGz50cJA/6MDlXNmOaZKrKe6su2sxTSktjCz99m9iHK CW0vBlpc/fVnQQW3iPG2Vies+rE5dkygoc/Rr8OdhnMiWgNWZp1i7rU5kKELQev8 bXUF1HZ85PM2ZpIztHtLKDQyDx07sQPyfEkZP6jSiQE08j1lIpFlqmNbGgreZcDO nNB2lPjZBnDOQpidIG96jbAFRQC8NkBTjW3kg3HxJJwqQVejP+WLJ+p9WRiPdOA5 bEHItX4tQvbHl0EK7/9uxHfEnzyZYSjkDl1pGNExMrNT8d03jQYj5iIrjl5twP6B vZMLZHJwOAvVRolIl3fwmUr1s6KHk9Akd3C1yuJb4o0kDxRA0kAJfSjpRx3NyiVo kbaXgXG514fdpPH1UfMoAq1oN74hyypbg73EqGUDLou6kqfbbzBRj9+dXWjRHQRa fvninTfv4invq0JOd1/Pn+uixWXwhpaMZtDngyjXRbz8DqlVp0B1chEGcZh7TzCs +n+gB21d =fl9b -----END PGP SIGNATURE----- --oTHb8nViIGeoXxdp--