[PATCH v2] efi_loader: Get rid of kaslr-seed

[Heinrich Schuchardt]

On 12/17/21 12:33, Mark Kettenis wrote:
>> Date: Fri, 17 Dec 2021 13:23:59 +0200
>> From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>
>> Hi Mark,
>>
>> On Fri, Dec 17, 2021 at 12:13:20PM +0100, Mark Kettenis wrote:
>>>> From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>>> Date: Fri, 17 Dec 2021 09:06:44 +0200
>>>>
>>>> Right now we unconditionally pass a 'kaslr-seed' property to the kernel
>>>> if the DTB we ended up in EFI includes the entry.  However the kernel
>>>> EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL for
>>>> it's own randomness needs (i.e the randomization of the physical
>>>> placement of the kernel).
>>>> So let's get rid of it if EFI_RNG_PPROTOCOL is installed.
>>>>
>>>> It's worth noting that TPMs also provide an RNG.  So if we tweak our
>>>> EFI_RNG_PROTOCOL slightly and install the protocol when a TPM device
>>>> is present the 'kaslr-seed' property will always be removed, allowing
>>>> us to reliably measure our DTB as well.
>>>
>>> That looks good to me.  But I think you should be honest here.  You're
>>> removing the kaslr-seed property here because it is a part of the DTB
>>> that changes on every boot, which interferes with the measurability.
>>> Not because there is something wrong with passing along the
>>> kaslr-seed.
>>
>> Yep never tried to hide that :)
>>
>>> In the absence of EFI_RNG_PROTOCOL support passing
>>> kaslr-seed still has a benefit as it will still be used for
>>> randomization of the virtual placement of the kernel.  So I think the
>>> comment could use some rewording (but the bit on randomization of the
>>> physical placement is certainly accurate and worth keeping).
>>>
>>> Oh, and please say "The Linux kernel's EFI STUB".
>>
>> Sure something along the lines of:
>>
>> "Right now we unconditionally keep a 'kaslr-seed' property in the DTB,
>> if what we ended up in EFI includes the entry.  However the Linux kernel
>> EFI-stub completely ignores it and only relies on EFI_RNG_PROTOCOL for
>> it's own randomness needs (i.e the randomization of the physical
>> placement of the kernel).  In fact it (blindly) overwrites the existing
>> seed if the EFI_RNG_PROTOCOL protocol is installed.  However it still uses
>> it for randomization of the virtual placement of the kernel if the EFI
>> protocol is not installed.
>> So let's get rid of it if EFI_RNG_PPROTOCOL is installed.
>>
>> It's worth noting that TPMs also provide an RNG.  So if we tweak our
>> EFI_RNG_PROTOCOL slightly and install the protocol when a TPM device
>> is present the 'kaslr-seed' property will always be removed, allowing
>> us to reliably measure our DTB as well."
>
> I would go in the other direction.  I'd explain why you want to remove
> kaslr-seed first (Property changes every boot; interferes with
> measured boot) and then explain why removing it in the presence of
> EFI_RNG_PROTOCOL support is fine (Linux kernel EFI STUB doesn't use
> for randomizing physical placement; overwrites it if EFI_RNG_PROTOCOL
> is implemented).
>
> I think describing it that way will prevent confusion in the future.
>
> Cheers,
>
> Mark
>
>>> Cheers,
>>>
>>> Mark
>>>
>>>> Acked-by: Ard Biesheuvel <ardb at kernel.org>
>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>>> ---
>>>> changes since v1:
>>>> - Only removing the property if EFI_RNG_PROTOCOL is installed, since some
>>>>    OS'es rely on kaslr-seed
>>>> - Only display an error message if the kaslr-seed entry was found but not
>>>>    removed
>>>>   cmd/bootefi.c                 |  2 ++
>>>>   include/efi_loader.h          |  2 ++
>>>>   lib/efi_loader/efi_dt_fixup.c | 33 +++++++++++++++++++++++++++++++++
>>>>   3 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>> index d77d3b6e943d..57f13ce701ec 100644
>>>> --- a/cmd/bootefi.c
>>>> +++ b/cmd/bootefi.c
>>>> @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt)
>>>>   	/* Create memory reservations as indicated by the device tree */
>>>>   	efi_carve_out_dt_rsv(fdt);
>>>>
>>>> +	efi_try_purge_kaslr_seed(fdt);

This function should only be invoked for CONFIG_EFI_TCG2_PROTOCOL=y.

>>>> +
>>>>   	/* Install device tree as UEFI table */
>>>>   	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
>>>>   	if (ret != EFI_SUCCESS) {
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 9dd6c2033634..1fe003db69e0 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
>>>>   					void **address);
>>>>   /* Carve out DT reserved memory ranges */
>>>>   void efi_carve_out_dt_rsv(void *fdt);
>>>> +/* Purge unused kaslr-seed */
>>>> +void efi_try_purge_kaslr_seed(void *fdt);
>>>>   /* Called by bootefi to make console interface available */
>>>>   efi_status_t efi_console_register(void);
>>>>   /* Called by bootefi to make all disk storage accessible as EFI objects */
>>>> diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c
>>>> index b6fe5d2e5a34..d3923e5dba1b 100644
>>>> --- a/lib/efi_loader/efi_dt_fixup.c
>>>> +++ b/lib/efi_loader/efi_dt_fixup.c
>>>> @@ -8,6 +8,7 @@
>>>>   #include <common.h>
>>>>   #include <efi_dt_fixup.h>
>>>>   #include <efi_loader.h>
>>>> +#include <efi_rng.h>
>>>>   #include <fdtdec.h>
>>>>   #include <mapmem.h>
>>>>
>>>> @@ -40,6 +41,38 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
>>>>   			addr, size);
>>>>   }
>>>>
>>>> +/**
>>>> + * efi_try_purge_kaslr_seed() - Remove unused kaslr-seed
>>>> + *
>>>> + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization
>>>> + * and completely ignores the kaslr-seed for its own randomness needs
>>>> + * (i.e the randomization of the physical placement of the kernel).
>>>> + * Weed it out from the DTB we hand over, which would mess up our DTB
>>>> + * TPM measurements as well.
>>>> + *
>>>> + * @fdt: Pointer to device tree
>>>> + */
>>>> +void efi_try_purge_kaslr_seed(void *fdt)
>>>> +{
>>>> +	const efi_guid_t efi_guid_rng_protocol = EFI_RNG_PROTOCOL_GUID;

There is not need to check if the RNG protocol is installed. If
CONFIG_EFI_TCG2_PROTOCOL=y, you should unconditionally remove
'kaslr-seed' as it is incompatible with measured boot.

Best regards

Heinrich

>>>> +	struct efi_handler *handler;
>>>> +	efi_status_t ret;
>>>> +	int nodeoff = 0;
>>>> +	int err = 0;
>>>> +
>>>> +	ret = efi_search_protocol(efi_root, &efi_guid_rng_protocol, &handler);
>>>> +	if (ret != EFI_SUCCESS)
>>>> +		return;
>>>> +
>>>> +	nodeoff = fdt_path_offset(fdt, "/chosen");
>>>> +	if (nodeoff < 0)
>>>> +		return;
>>>> +
>>>> +	err = fdt_delprop(fdt, nodeoff, "kaslr-seed");
>>>> +	if (err < 0 && err != -FDT_ERR_NOTFOUND)
>>>> +		log_err("Error deleting kaslr-seed\n");
>>>> +}
>>>> +
>>>>   /**
>>>>    * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>>>>    *
>>>> --
>>>> 2.30.2
>>>>
>>>>
>>