From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 966A0C433F5 for ; Fri, 17 Dec 2021 11:33:41 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A7B7A83045; Fri, 17 Dec 2021 12:33:38 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 9EFC483063; Fri, 17 Dec 2021 12:33:36 +0100 (CET) Received: from sibelius.xs4all.nl (sibelius.xs4all.nl [83.163.83.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 15F358303B for ; Fri, 17 Dec 2021 12:33:33 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=mark.kettenis@xs4all.nl Received: from localhost (bloch.sibelius.xs4all.nl [local]) by bloch.sibelius.xs4all.nl (OpenSMTPD) with ESMTPA id 45c5397c; Fri, 17 Dec 2021 12:33:32 +0100 (CET) Date: Fri, 17 Dec 2021 12:33:32 +0100 (CET) From: Mark Kettenis To: Ilias Apalodimas Cc: xypron.glpk@gmx.de, ardb@kernel.org, agraf@csgraf.de, u-boot@lists.denx.de In-Reply-To: (message from Ilias Apalodimas on Fri, 17 Dec 2021 13:23:59 +0200) Subject: Re: [PATCH v2] efi_loader: Get rid of kaslr-seed References: <20211217070644.2458603-1-ilias.apalodimas@linaro.org> Message-ID: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean > Date: Fri, 17 Dec 2021 13:23:59 +0200 > From: Ilias Apalodimas > > Hi Mark, > > On Fri, Dec 17, 2021 at 12:13:20PM +0100, Mark Kettenis wrote: > > > From: Ilias Apalodimas > > > 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 > > > Signed-off-by: Ilias Apalodimas > > > --- > > > 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); > > > + > > > /* 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 > > > #include > > > #include > > > +#include > > > #include > > > #include > > > > > > @@ -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; > > > + 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 > > > > > > >