[PATCH] efi_loader: FMP cleanups

[Ilias Apalodimas]

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.

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

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?

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

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

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