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