Pull request for efi-2021-10-rc4

Simon Glass sjg at chromium.org
Thu Sep 9 10:56:51 CEST 2021 

Hi Ilias,

On Sat, 4 Sept 2021 at 14:09, Ilias Apalodimas
<ilias.apalodimas at linaro.org> 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.
> >
> > 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.
> 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
> 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.

This is a reasonable summary I think.

The devicetree series I sent explains how to deal with the limitations
here. Heinrich requested that I clarify this which I did. I fully
expected that the revert would then be applied. But instead it seems
there is not even agreement about the status quo (of use of devicetree
in U-Boot).

OF_SEPARATE is used by the vast majority of boards, including most
qemu builds. I think the OF_BOARD thing should probably be deleted.
The OF_EMBED thing should not be used in production. It is needed with
the EFI app though and I recently sent a series to support updating
the DT there.

For OF_PRIOR_STAGE the prior state is responsible for supplying the
DT, and needs to do so and meet U-Boot's requirements. I have clearly
set that out in the devicetree patch.

https://patchwork.ozlabs.org/project/uboot/list/?series=260032&state=*

So what has happened here is a short-cut and not the correct approach.
It needs to be reverted before the release, since otherwise people
will rely on it and we will be stuck with two ways to do signatures.

Regards,
Simon

More information about the U-Boot mailing list