Pull request for efi-2021-10-rc4

Mon Sep 6 02:47:57 CEST 2021

On Sat, Sep 04, 2021 at 11:08:28PM +0300, Ilias Apalodimas wrote:
> Hi Tom,
> 
> On Sat, 4 Sept 2021 at 21:08, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sat, Sep 04, 2021 at 08:02:49PM +0200, Heinrich Schuchardt wrote:
> > >
> > >
> > > Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini <trini at konsulko.com>:
> > > >On Sat, Sep 04, 2021 at 07:03:48PM +0200, Heinrich Schuchardt wrote:
> > > >>
> > > >>
> > > >> Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini <trini at konsulko.com>:
> > > >> >On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote:
> > > >> >>
> > > >> >>
> > > >> >> Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini <trini at konsulko.com>:
> > > >> >> >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote:
> > > >> >> >
> > > >> >> >> Dear Tom,
> > > >> >> >>
> > > >> >> >> The following changes since commit 94509b79b13e69c209199af0757afbde8d2ebd6d:
> > > >> >> >>
> > > >> >> >>   btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24
> > > >> >> >> -0400)
> > > >> >> >>
> > > >> >> >> are available in the Git repository at:
> > > >> >> >>
> > > >> >> >>   https://source.denx.de/u-boot/custodians/u-boot-efi.git
> > > >> >> >> tags/efi-2021-10-rc4
> > > >> >> >>
> > > >> >> >> for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324:
> > > >> >> >>
> > > >> >> >>   efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
> > > >> >> >> (2021-09-04 09:15:09 +0200)
> > > >> >> >>
> > > >> >> >> ----------------------------------------------------------------
> > > >> >> >> Pull request for efi-2021-10-rc4
> > > >> >> >>
> > > >> >> >> Documentation:
> > > >> >> >>
> > > >> >> >>     Remove invalid reference to configuration variable in UEFI doc
> > > >> >> >>
> > > >> >> >> UEFI:
> > > >> >> >>
> > > >> >> >>     Parameter checks for the EFI_TCG2_PROTOCOL
> > > >> >> >>     Improve support of preseeding UEFI variables.
> > > >> >> >>     Correct the calculation of the size of loaded images.
> > > >> >> >>     Allow for UEFI images with zero VirtualSize
> > > >> >> >>
> > > >> >> >> ----------------------------------------------------------------
> > > >> >> >> Heinrich Schuchardt (5):
> > > >> >> >>       efi_loader: sections with zero VirtualSize
> > > >> >> >>       efi_loader: rounding of image size
> > > >> >> >>       efi_loader: don't load signature database from file
> > > >> >> >>       efi_loader: efi_auth_var_type for AuditMode, DeployedMode
> > > >> >> >>       efi_loader: correct determination of secure boot state
> > > >> >> >>
> > > >> >> >> Masahisa Kojima (3):
> > > >> >> >>       efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api
> > > >> >> >>       efi_loader: fix boot_service_capability_min calculation
> > > >> >> >>       efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check
> > > >> >> >
> > > >> >> >And I don't see Simon's revert in here either.  And he asked you about
> > > >> >> >that yesterday:
> > > >> >> >https://lore.kernel.org/r/CAPnjgZ3eRdjF0jb9S-cJK6y+feuyRyWf0hNkf2triB4DR4UFBQ@mail.gmail.com/
> > > >> >> >
> > > >> >> >So at this point, are you asserting there is nothing to revert?
> > > >> >>
> > > >> >> Never. Simons "revert" is breaking functionality. The concept for suporting blobs in devicetrees supplied by a prior bootstage has not been defined yet.
> > > >> >
> > > >> >And to be clearer, reverting something that was introduced in one rc in
> > > >> >a later rc isn't breaking functionality.  U-Boot releases (well, the
> > > >> >non-rc ones for sure) are on a very regular schedule.  External projects
> > > >> >may not depend on some feature introduced at -rcN unless they're willing
> > > >> >to accept that some changes could happen before release.
> > > >> >
> > > >>
> > > >> There is no value delivered by Simon's series. Neither does the image get smaller nor does it fix anything. If he wants to enforce a design, it must work for all use cases. But this requires some conceptual work.
> > > >
> > > >Yes, and what's the rush to not do the conceptual work?  If I recall
> > > >part of the thread correctly, yes, Simon didn't get his objections in
> > > >before the patches were merged, but it was early enough in the release
> > > >cycle that taking a step back and reverting was a reasonable request.
> > > >What he had said wouldn't have changed if he had gotten the email out a
> > > >few days earlier.
> > > >
> > > >So yes, please merge Simon's revert, or post and merge new more minimal
> > > >revert that brings things to the same functional end.  There are
> > > >objections to this implementation, and thus far Simon has been
> > > >responding all of the requests to better clarify all of the related code
> > > >and concepts that have been asked of him, so that in the end an
> > > >implementation that fulfills all of the technical requirements can be
> > > >created, that hopefully leaves all parties satisfied.
> > >
> > > There is nothing wrong with the current code.
> > >
> > > It is Simon's concept of blobs in devicetrees that is borked in that
> > > it ignores QEMU and any board that gets the DT from a prior boot
> > > stage.

But it won't prevent the other systems from using their device trees.

> > Then it should be pretty easy to get Simon to withdraw his objections,
> > if there's such a fundamental "this is the only possible way, no
> > changes" path forward.
> >
> > > Simon's patches have no functional end. So what do you mean by "same functional end"?
> >
> > I mean the state of the EFI subsystem, prior to the code in question
> > being merged, without breaking the other assorted EFI changes that have
> > come in since then.
> >
> > --
> > Tom
> I'll sum this up since there's many emails on the topic.
> 
> The current changes move the public needed for capsule updates in
> U-Boot's .rodata section.
> When I sent this, I assumed u-boot was mapping .rodata as read only.
> Since it doesn't the protection I was hoping for isn't there. So
> security wise the two different proposals are on par.  Arguably it's
> easier to fix .rodata instead of copying the key from the dtb and
> switching the pages to RO, but that's really minor.
> 
> However keeping the key on the DTB has some of limitations, with the
> most notable being that you *must* only use CONFIG_OF_SEPARATE for
> your DTB, while there's four different ways available in the Kconfig
> (and 3 are usable on production).  I've repeated enough times, that I
> don't mind changing the code and keeping the key on the DTB, as long
> as the limitations are lifted.  If that means reverting the patch now
> and fixing it in the future, I am fine with that as well.

Different people have different requirements/assumptions on
different systems. It's also true for firmware update.
Some might like Ilias' approach, others are satisfied with device tree.

I think that what we need to do is, instead of having a single solution,
to allow users to decide which method, or even their own one,
they want to use on their systems. Right?

> To be honest I don't understand why this has to be set in stone.  Even
> if we keep the current patchset and change it to the dtb in the
> future, that will have zero effect on the users. Once they upgrade to

Surely it is system admin/integrators, not users, who care here.

-Takahiro Akashi

> the newer, shinier version, their key will just be read from a
> different location, but that's all hidden from the user. The only they
> will have to change, is how they include that key on the final binary.
> 
> Regards
> /Ilias