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=-5.8 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 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 53CCAC48BE5 for ; Tue, 15 Jun 2021 06:40:40 +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 43E3A6140F for ; Tue, 15 Jun 2021 06:40:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 43E3A6140F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id CA770803B2; Tue, 15 Jun 2021 08:40:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="aLYJDfp9"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D088A82845; Tue, 15 Jun 2021 08:40:35 +0200 (CEST) Received: from mail-pg1-x531.google.com (mail-pg1-x531.google.com [IPv6:2607:f8b0:4864:20::531]) (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 7FCA2801BE for ; Tue, 15 Jun 2021 08:40:31 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pg1-x531.google.com with SMTP id e22so10601731pgv.10 for ; Mon, 14 Jun 2021 23:40:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=AR600C9cIskfzlXwU93QT1/6Uj/tmCyveN6mdBurFa8=; b=aLYJDfp90dRHSclqEHjjP0ItUDQdXdU//9P2J/GZZu1/8UDHN9HOJYH3r+7Y0HuE77 yDk7UXI8/rkDtn4AVgArsa7woWlHPFf3Eylv26p3JeAQ2o6Wj/6N1r6P8K1B6J7wjMB/ K7g0/5jUqikcMekmbonFDBoWhb40nuly9VMQ8mlRD8HNiVK5W0mOrxZlcyfT/sQzMyvV obPHi1vLtzvMjVLr4J2vwOilXbtoubyo4SRFPvISH91crK+2UpvG3PknLdKeKlUmZk7p /PKOfsG7QvPl91oBPlWV+3OlzNDQNqHAtDeFP03akXiJoTlD4K87ATwU6tKCatv8WjzF Eu8g== 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 :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=AR600C9cIskfzlXwU93QT1/6Uj/tmCyveN6mdBurFa8=; b=TStGoOep1De10ZUKKHRDeXXO73/EJYrYfKiRLkZQ81F7xEkWP+7p2Xnc6FcLHEBf7j RwtSYgVYNO9Hk4O5pmBM6We7UM5SrFeKazp/kFullXf1df/1ThKWoA71SGQcljE0lPda RsYbStVh2ovvwQsPgEgl87wz00QPrti9/V/bsReRWi6N8dGwS+vnsQ5nmhJCfrSLqFGF zNV3cyFb4/Dkdy3aCMO+Dn7g5O/0s5tmCfFDpk8w0nmTTWV2MH1TKaew+oj+KvNvT8aO 3OkieIvry/CDak6rS7Dt587bUbbcvutBo8BWWeTmcwbHAlDFA4Nz5lHpeR9/cquNn5Ab y/2w== X-Gm-Message-State: AOAM532YRbqYuH9r9D22JYnhOqj4N06bYg4gLexPOgVrFnWH5D/34fHh jsifZ7SiGSx2rOYchRyHA1kceg== X-Google-Smtp-Source: ABdhPJzWQ8IZyr89qF5lNvha+CzvAxscy+Mfuca2OxO1P0Lg7wG5aO4squO9gK/dzMVlRJNk+XuyTg== X-Received: by 2002:a63:9302:: with SMTP id b2mr1383780pge.277.1623739228130; Mon, 14 Jun 2021 23:40:28 -0700 (PDT) Received: from laputa (p3dd30534.tkyea130.ap.so-net.ne.jp. [61.211.5.52]) by smtp.gmail.com with ESMTPSA id w142sm14806816pff.154.2021.06.14.23.40.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 14 Jun 2021 23:40:27 -0700 (PDT) Date: Tue, 15 Jun 2021 15:40:23 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: xypron.glpk@gmx.de, masami.hiramatsu@linaro.org, Simon Glass , Mario Six , Michal Simek , Alexander Graf , u-boot@lists.denx.de Subject: Re: [PATCH] efi_loader: FMP cleanups Message-ID: <20210615064023.GA58428@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , xypron.glpk@gmx.de, masami.hiramatsu@linaro.org, Simon Glass , Mario Six , Michal Simek , Alexander Graf , u-boot@lists.denx.de References: <20210614151015.99992-1-ilias.apalodimas@linaro.org> <20210615015101.GA46087@laputa> <20210615044458.GA54329@laputa> <20210615055538.GA56793@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 On Tue, Jun 15, 2021 at 09:22:31AM +0300, Ilias Apalodimas wrote: > On Tue, Jun 15, 2021 at 02:55:38PM +0900, AKASHI Takahiro wrote: > > On Tue, Jun 15, 2021 at 08:23:35AM +0300, Ilias Apalodimas wrote: > > > On Tue, Jun 15, 2021 at 01:44:58PM +0900, AKASHI Takahiro wrote: > > > > On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote: > > > > > Akashi-san, > > > > > > > > > > On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote: > > > > > > Ilias, > > > > > > > > > > > > In this patch, you are trying to address a couple of independent > > > > > > issues in a single commit. > > > > > > Please split. > > > > > > (Heinrich doesn't like that.) > > > > > > > > Any comment? > > > > > > They are fixing the ESRT table generation, while cleaning up what's already in > > > there. Besides Heinrich can comment himself if he wants them split or not. > > > > They are fixing "different" problems relating ESRT generation. > > That is my point. > > > > Sure, but it's a minor clean up really. As I said the current code works > fine. So I dont really mind the fact that it breaks a sentence of the spec. > Hence I considered the cleanup and the mutual exclusive part to be really > minor. Yes, it's minor but still a different problem. Let me give you an example. If I correct a misspelling in a given code very close to the change, Heinrich would ask me to add a separate patch as it is simply not related. Moreover, from the viewpoint of maintenance (i.e. bisect ability), they should be separated from each other. > > > > > > > > > > > > > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote: > > > > > > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at > > > > > > > the same time. Moreover we only install those if a CapsuleUpdate is > > > > > > > requested. Since we now have an ESRT table, it makes more sense to > > > > > > > unconditionally install the FMP, so any userspace applications (e.g fwupd) > > > > > > > can make use of them and trigger an update. > > > > > > > > > > > > > > While at it clean up the FMP installation as well. Chapter 23 of the EFI > > > > > > > spec (rev 2.9) says: > > > > > > > "A specific updatable hardware firmware store must be represented by > > > > > > > exactly one FMP instance". > > > > > > > This is not the case for us, since both of our FMP protocols can be > > > > > > > installed at the same time and are controlled by a single 'dfu_alt_info' > > > > > > > env variable. > > > > > > > So make the config option a choice and allow the user to install one > > > > > > > of them at any given time. > > > > > > > > > > > > I'd like to say nak in some respects: > > > > > > - Although I do understand the UEFI requirement that you mentioned above, > > > > > > FIT and RAW FMP drivers can handle *different* firmware even though > > > > > > they share the same dfu_alt_info. > > > > > > > > > > How ? > > > > > > > > One idea that I can imagine is that we may be able to utilize > > > > "update_image_index", which is currently not used effectively, > > > > in order to specify which firmware in dfu_alt_info be handled > > > > by either FIT FMP or RAW FMP. > > > > > > So it's not being used right now, and the fact is they are at the moment doing > > > the same thing. And even if it does, no one in his right mind will create a > > > platform and say "Hey let me create half of the capsules as raw and the rest > > > of them as FIT, it would be fun to watch users struggle". > > > > You misunderstand me. > > Because you asked me about an idea about how to fix the issue, > > I answered to it. I have never said that the current code does not > > have a problem that you mentioned. > > So I said: > > > > > > We should not impose unnecessary restriction if we manage to have some > > > > > > workaround to meet the requirement. > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > I was mostly asking for existing code that I might have missed in my greps, > but I get the idea. > > > > > > Is there anything very specific that you can achieve with FIT capsules that > > > you can't achieve with RAW ones (or vice versa), that would justify having > > > them both present at the same time? > > > > Yes. > > We may have different *firmware* for different software components > > and different devices. For example, > > You have firmare like U-Boot binary and default variable storage > > in different partitions. > > On the other hand, you have an extra firmware for a particular > > peripheral, like PCI device or anything else, which comes > > from a 3rd party vendor of the device. > > The former may and can be packed into a single binary in FIT format. > > The latter can be used in a separate RAW format as the timing of > > updating those firmware is likely to be different. > > > > Sure that's a use case. But that's not a specific one, nor something you cant > do without both of them being installed. You can arguably just create a RAW > image for the second firmware and put the info into dfu_alt_info. Why do you stick to a single format? We can reasonably assume that each FMP may have a different format. I think it's a very natural thing. > So unless we > have an example of a device that says "This firmware file can only be updated > by a FIT image, while the rest of the firmware is on a FAT filesystem", I don't > see any reason why we need to support that. The changes are not set in stone > anyway. The code was fine before the ESRT got involved. So all my patch > really does is make the current code useful when an ESRT is installed. We can > then break the spec on purpose (yes break it :>) ignore the OsIndications > bit and have fwupd working with U-Boot. This will have an actual impact on > devices and the code usability, since people will start using it. I prefer > this over adding a very cumbersome corner case, that's arguably no one will > ever need. > We can always go back and make them a config option in the future. But unless > we get a use case for it, I'd still prefer having them mutually exclusive, > rather than adding code for an imaginary device (which I really doubt anyone > will ever design). I don't think that the example I gave is a imaginary device. > > > > > > > > > > We should not impose unnecessary restriction if we manage to have some > > > > > > workaround to meet the requirement. > > > > > > > > > > It's not the updating part only. It's that the .get_image_info also relies on > > > > > the same env variable. > > > > > > > > The above idea can and should be applied to GetImageInfo implementation > > > > at the same time. > > > > > > Yes but can you do it with just changing the env variable now? Or you need to > > > add more code into the DFU logic? > > > > Those *meta* data for firmware can be declared/specified outside of FMP, > > and be referred to by FMP (and/or ESRT). That is what I meant by: > > > > > > (I still believe that the firmware definition for ESRT should exist > > > > > > elsewhere other than in FMP themselves.) > > > > > > > > > > > > > Specifically in the fwupd case on an RPI4 with the > > > > > dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were > > > > > populated only one was reported. Probably because this really does give you > > > > > 2 ways of updating the same flash. > > > > > > > > > > > (I still believe that the firmware definition for ESRT should exist > > > > > > elsewhere other than in FMP themselves.) > > > > > > > > > > That's a whole different story, and if we have that, then .get_image_info > > > > > should change as well instead of using the DFU information. > > > > > > > > I don't think so as I mentioned above. > > > > > > And I don't see any benefit from storing the same information in 2 completely > > > disjoint entities. > > > > ? > > The ESRT code right now uses get_image_info from the FMP code and the FMP code > uses the dfu_alt_info to derive whatever information it needs. Both of these > concepts are trying to provide information about the running firmware. So if > we change that imho both of them should get that info from an abstracted > object (file/c struct in u-boot/whatever). But really I think using FMP to > fill ESRT entries is fine (at least for me). Well, dfu_alt_info can already be seen as abstracted object in terms of FMP. > > > > > > > > > > > Because right > > > > > now we enabled security (or think we have), while allowing users to set an env > > > > > variable which is not authenticated, and completely change what the > > > > > firmware reports (or updates). > > > > > > > > This is the point that I mentioned earlier in our private chat, > > > > and it's a "whole different" story in this context. > > > > > > You mentioned that in the context of "can we install the FMPs during the EFI > > > init". Since the variable is interpreted at runtime, we definitely can. I > > > looked back at that chat and saw nothing related to the security problems > > > we'll create. > > > > You have referred to this issue in the context of security. > > So I said that it was a different story. > > The issue that you're trying to address in this patch is *NOT* security. > > > > No it's not, I am just pointing out the obvious. > > > > In any case the problem here is real, but there are sane ways to avoid it. > > > > > > > > > > > > We can always add a huge warning saying > > > > > something along the lines of "If you really care this should come with a > > > > > CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1". > > > > > > > > > > The spec is pretty clear and we allow users to *break* it with a config > > > > > option. Arguably this is fine, since the code continues to work fine and > > > > > you can perform the updates, but in essence RAW and FITs are used to update > > > > > the same medium right now. You can't have a capsule with half it's contents > > > > > describing something RAW and the other half being a FIT. You have a FIT based > > > > > capsule or a RAW based capsule. > > > > > > > > See above. > > > > > > I still don't get it. > > > The fact is we have a config option, that if the user decides to set in a > > > specific way (and that specific way is 99% of the use cases) we'll break the > > > EFI spec. > > > > As I said above, I have never said that the current implementation does > > not break EFI spec if not properly used. > > So I suggested a possible solution in the previous email as you asked me. > > > > > So unless we add code into the dfu logic, parsing dfu_alt_info and > > > figuring out if the user is allowed to do that or not, I really think those two > > > must be treated as mutually exclusive. > > > > I don't think that we need to modify DFU code. > > Yea me neither, but since the firmware runtime information are derived from > that, we don't have that many options. What do you mean by "options"? -Takahiro Akashi > Thanks > /Ilias > > > > -Takahiro Akashi > > > > > > > > > > > > - We should allow users to add their own FMP drivers and so not call > > > > > > [arch_]efi_load_capsule_drivers() unconditionally > > > > > > even if you don't like "__weak" attribute. > > > > > > > > > > I am fine with the __weak attribute. On the other hand I consider the > > > > > current code the defacto way users would use to update their firmware. That's > > > > > why I removed the __weak attribute. If a hardware vendor was to update > > > > > their special PCI option ROM or a flash that lives on the secure world they > > > > > should install their FMPs on a different handle and leave the current code > > > > > as is. > > > > > > > > And we should provide an option that disables these existing handle. > > > > > > The existing one is not enough? > > > > > > > > > > > > > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will > > > > > > leave some test cases in pytest skipped. > > > > > > > > > > Yea that's unfortunate, but maybe we can just add an extra config on the > > > > > sandbox? > > > > > > > > Please add another patch that is missing. > > > > > > > > > > > > > > -Takahiro Akashi > > > >