[PATCH] efi_loader: FMP cleanups

Tue Jun 15 08:22:31 CEST 2021

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. 

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

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

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

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