Pull request for efi-2021-10-rc4

Thu Sep 9 13:21:17 CEST 2021

On 9/9/21 10:56 AM, Simon Glass wrote:
> 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.

Yes, and this was wrong. You cannot impose U-Boot's requirements on
other projects.

Instead I suggested to use an overlay tree to add our stuff on the
devicetree from the prior stage once U-Boot is invoked.

Best regards

Heinrich

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