[PATCH] efi_loader: FMP cleanups

[AKASHI Takahiro]

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
> > > >