[PATCH 1/1] efi_leader: delete rng-seed if having EFI RNG protocol

[Heinrich Schuchardt]

On 20.09.24 17:58, Simon Glass wrote:
> Hi Ilias,
> 
> On Fri, 20 Sept 2024 at 09:37, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On Fri, 20 Sept 2024 at 10:25, Simon Glass <sjg at chromium.org> wrote:
>>>
>>> Hi Ilias,
>>>
>>> On Thu, 19 Sept 2024 at 17:51, Ilias Apalodimas
>>> <ilias.apalodimas at linaro.org> wrote:
>>>>
>>>> On Thu, 19 Sept 2024 at 18:39, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Thu, 19 Sept 2024 at 17:37, Ilias Apalodimas
>>>>> <ilias.apalodimas at linaro.org> wrote:
>>>>>>
>>>>>> On Thu, 19 Sept 2024 at 18:19, Simon Glass <sjg at chromium.org> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, 19 Sept 2024 at 17:13, Ilias Apalodimas
>>>>>>> <ilias.apalodimas at linaro.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Sep 19, 2024, 18:05 Heinrich Schuchardt <heinrich.schuchardt at canonical.com> wrote:
>>>>>>>>>
>>>>>>>>> On 19.09.24 17:00, Simon Glass wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On Thu, 19 Sept 2024 at 16:32, Ilias Apalodimas
>>>>>>>>>> <ilias.apalodimas at linaro.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> On Thu, 19 Sept 2024 at 17:20, Heinrich Schuchardt
>>>>>>>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 19.09.24 16:10, Simon Glass wrote:
>>>>>>>>>>>>> Hi Heinrich,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sat, 14 Sept 2024 at 18:06, Heinrich Schuchardt
>>>>>>>>>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> For measured be boot we must avoid any volatile values in the device-tree.
>>>>>>>>>>>>>> We already delete /chosen/kaslr-seed if we provide and EFI RNG protocol.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Could you explain a bit why this is, and where this is checked?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Additionally remove /chosen/rng-seed provided by QEMU or U-Boot.
>>>>>>>>>>>>
>>>>>>>>>>>> Measured boot relies on creating hashes of artifacts and writing these
>>>>>>>>>>>> to TPM. If the hashes don't match the OS will either warn or refuse to
>>>>>>>>>>>> boot. The device-tree is one of the artifacts that are measured.
>>>>>>>>>>>>
>>>>>>>>>>>> If we have random values in /chosen, measured boot will fail.
>>>>>>>>>>>>
>>>>>>>>>>>> When an EFI RNG protocol is provided by the firmware, GRUB and the
>>>>>>>>>>>> kernel will use it instead of /chosen/rng-seed and /chosen/kaslr-seed.
>>>>>>>>>>>
>>>>>>>>>>> There's a comment on top of that function that explains what happens as well.
>>>>>>>>>>> In short the EFI stub does not even look at the KASLR seed and never
>>>>>>>>>>> randomizes the physical placement of the kernel. It only does that
>>>>>>>>>>> when the EFI_RNG protocol is there.
>>>>>>>>>>
>>>>>>>>>> OK thank you. I suppose I am more just wondering why it got added in
>>>>>>>>>> the first place?
>>>>>>>>>
>>>>>>>>> For booting via the legacy Linux entry point adding kaslr-seed allows to
>>>>>>>>> randomize addresses. QEMU adds rng-seed instead of kaslr-seed.
>>>>>>>>
>>>>>>>>
>>>>>>>> Not the kernel physical placement. It randomizes only the virtual placement
>>>>>>>
>>>>>>> So, are you saying that U-Boot adds this field into the FDT and then removes it?
>>>>>>
>>>>>> Yes. As Heinrich said, the rng seed is still usable for some
>>>>>> randomization. If we boot with EFI and have an RNG protocol, we dont
>>>>>> need it and it also messes up the TPM measurements, so we remove it.
>>>>>> But the code that injects it to u-boot, or the prior bootloader that
>>>>>> handed you over a DT,  does not know if you plan to boot with EFI.
>>>>>
>>>>> I'm actually surprised that this works. Normally, removing a property
>>>>> does not drop that property from the string table, so adding a
>>>>> property and deleting it is not normally the same as never adding it.
>>>>> But perhaps that has changed?
>>>>>
>>>>> Anyway, I think we should add the property when we know it is OK to do
>>>>> so, which is just before we boot. If you agree I can take a look at
>>>>> that.
>>>>
>>>> Sure, but we won't be able to remove this code. There are still
>>>> first-stage boot loaders that might send you a DT over a bloblist,
>>>> that has a kaslr-seed
>>>
>>> But then I don't see what problem that causes...I'm just going to
>>> leave this madness to you :-)
>>>
>>
>> The problem is that in theory, we need to measure a DT as early as
>> possible, but that's not doable in U-Boot yet. The TCG specs have no
>> guidance on measuring DTs, but do have on ACPI tables and we basically
>> copy what's done for ACPI.
>>
>> The DT is more fragile though. As Heinrich said if you decide to
>> measure it, you can't have random values -- whether that comes from
>> U-Boot injecting those or a previous loader is irrelevant. The result
>> is the same, if you have random values in the measured DT, e.g a mac
>> address or a kaslr-seed the measurements we take on PCR1 will be
>> different and as a result kind of useless.
>>
>> For the reasons above, measuring a DT is not in any default config.
>> It's up to the user whether he wants to measure it or not. But for the
>> kaslr-seed, since the kernel does not use it when booting via EFI and
>> the EFI_RNG protocol is installed, we remove it.
>>
>> Hope that clarifies things a bit
> 
> Yes it does, thank you.
> 
> IMO measuring the devicetree should really be done using a hash of
> only part of the tree, for example excluding the /options node and
> perhaps /chosen as well. I just looked to refresh my memory, and
> adding then removing a property does not result in the same DT as
> before the addition.

We must not exclude the /chosen node as it may contain:

* /chosen/bootargs - Command line arguments must be measured.
* /chosen/initrd - Filename of the initial RAM disk.

For more items see 
https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt.

> 
> There are several reasons I have this opinion:
> - The DT is used for things which don't relate to Linux, such as /options/u-boot
> - The DT may have overlays applied
> - Some DT nodes may have been 'fixed' up by U-Boot
> 
> So if we want to 'measure' the DT, we should probably provide in the
> DT an indication of which part is hashed, so Linux etc. can take that
> into account. Also we may want to have multiple hashes, to deal with
> the case of additions being made. We have something similar to this
> with FIT signatures, but that mechanism is not suitable here (although
> it could perhaps help inspire a solution).

Current Linux just expects the measurement. It would not to be able to 
consume any information describing which part of the DT has been measured.

The idea of measurement is that it should capture the whole state. We 
cannot generally exclude fix-ups:

If memory is added and U-Boot changes the device-tree accordingly, this 
should change the measurement.

If the network card is replaced with a different card resulting in a 
different MAC address copied to the device-tree, this should change the 
measurement, too.

Best regards

Heinrich