[PATCH] efi: random: zero out secret after use and do not take minimum

[PATCH] efi: random: zero out secret after use and do not take minimum

[Jason A. Donenfeld]

Taking the minimum is wrong, if the bootloader or EFI stub is actually
passing on a bunch of bytes that it expects the kernel to hash itself.
Ideally, a bootloader will hash it for us, but STUB won't do that, so we
should map all the bytes. Also, all those bytes must be zeroed out after
use to preserve forward secrecy.

Fixes: 161a438d730d ("efi: random: reduce seed size to 32 bytes")
Cc: stable@vger.kernel.org # v4.14+
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/firmware/efi/efi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index f73709f7589a..819409b7b43b 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -630,7 +630,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 		seed = early_memremap(efi_rng_seed, sizeof(*seed));
 		if (seed != NULL) {
-			size = min(seed->size, EFI_RANDOM_SEED_SIZE);
+			size = seed->size;
 			early_memunmap(seed, sizeof(*seed));
 		} else {
 			pr_err("Could not map UEFI random seed!\n");
@@ -641,6 +641,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 			if (seed != NULL) {
 				pr_notice("seeding entropy pool\n");
 				add_bootloader_randomness(seed->bits, size);
+				memzero_explicit(seed->bits, size);
 				early_memunmap(seed, sizeof(*seed) + size);
 			} else {
 				pr_err("Could not map UEFI random seed!\n");
-- 
2.38.1

Re: [PATCH] efi: random: zero out secret after use and do not take minimum

[Ard Biesheuvel]

On Wed, 16 Nov 2022 at 21:06, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Taking the minimum is wrong, if the bootloader or EFI stub is actually
> passing on a bunch of bytes that it expects the kernel to hash itself.

We still need some kind of limit, just so things don't explode if
seed->size is bogus.

> Ideally, a bootloader will hash it for us, but STUB won't do that, so we
> should map all the bytes. Also, all those bytes must be zeroed out after
> use to preserve forward secrecy.
>
> Fixes: 161a438d730d ("efi: random: reduce seed size to 32 bytes")
> Cc: stable@vger.kernel.org # v4.14+
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/firmware/efi/efi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index f73709f7589a..819409b7b43b 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -630,7 +630,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>
>                 seed = early_memremap(efi_rng_seed, sizeof(*seed));
>                 if (seed != NULL) {
> -                       size = min(seed->size, EFI_RANDOM_SEED_SIZE);
> +                       size = seed->size;
>                         early_memunmap(seed, sizeof(*seed));
>                 } else {
>                         pr_err("Could not map UEFI random seed!\n");
> @@ -641,6 +641,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                         if (seed != NULL) {
>                                 pr_notice("seeding entropy pool\n");
>                                 add_bootloader_randomness(seed->bits, size);
> +                               memzero_explicit(seed->bits, size);
>                                 early_memunmap(seed, sizeof(*seed) + size);
>                         } else {
>                                 pr_err("Could not map UEFI random seed!\n");
> --
> 2.38.1
>

Re: [PATCH] efi: random: zero out secret after use and do not take minimum

[Jason A. Donenfeld]

On Wed, Nov 16, 2022 at 10:44:20PM +0100, Ard Biesheuvel wrote:
> On Wed, 16 Nov 2022 at 21:06, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Taking the minimum is wrong, if the bootloader or EFI stub is actually
> > passing on a bunch of bytes that it expects the kernel to hash itself.
> 
> We still need some kind of limit, just so things don't explode if
> seed->size is bogus.

Alright, let's just say 1k then. Will send a v2.

Jason

[PATCH v2] efi: random: zero out secret after use and do not take minimum

[Jason A. Donenfeld]

Taking the minimum is wrong, if the bootloader or EFI stub is actually
passing on a bunch of bytes that it expects the kernel to hash itself.
Ideally, a bootloader will hash it for us, but STUB won't do that, so we
should map all the bytes. Also, all those bytes must be zeroed out after
use to preserve forward secrecy.

Fixes: 161a438d730d ("efi: random: reduce seed size to 32 bytes")
Cc: stable@vger.kernel.org # v4.14+
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v1->v2:
- Cap size to 1k.
 drivers/firmware/efi/efi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index a46df5d1d094..c7c7178902c2 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -611,7 +611,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 		seed = early_memremap(efi_rng_seed, sizeof(*seed));
 		if (seed != NULL) {
-			size = min(seed->size, EFI_RANDOM_SEED_SIZE);
+			size = min_t(u32, SZ_1K, seed->size);
 			early_memunmap(seed, sizeof(*seed));
 		} else {
 			pr_err("Could not map UEFI random seed!\n");
@@ -622,6 +622,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 			if (seed != NULL) {
 				pr_notice("seeding entropy pool\n");
 				add_bootloader_randomness(seed->bits, size);
+				memzero_explicit(seed->bits, size);
 				early_memunmap(seed, sizeof(*seed) + size);
 			} else {
 				pr_err("Could not map UEFI random seed!\n");
-- 
2.38.1

Re: [PATCH v2] efi: random: zero out secret after use and do not take minimum

[Ard Biesheuvel]

On Thu, 17 Nov 2022 at 01:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Taking the minimum is wrong, if the bootloader or EFI stub is actually
> passing on a bunch of bytes that it expects the kernel to hash itself.
> Ideally, a bootloader will hash it for us, but STUB won't do that, so we
> should map all the bytes. Also, all those bytes must be zeroed out after
> use to preserve forward secrecy.
>
> Fixes: 161a438d730d ("efi: random: reduce seed size to 32 bytes")
> Cc: stable@vger.kernel.org # v4.14+
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v1->v2:
> - Cap size to 1k.
>  drivers/firmware/efi/efi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Thanks. I'll just incorporate this into the patch that does the
concatenation of seeds in the stub, which is queued up for v6.2

> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index a46df5d1d094..c7c7178902c2 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -611,7 +611,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>
>                 seed = early_memremap(efi_rng_seed, sizeof(*seed));
>                 if (seed != NULL) {
> -                       size = min(seed->size, EFI_RANDOM_SEED_SIZE);
> +                       size = min_t(u32, SZ_1K, seed->size);
>                         early_memunmap(seed, sizeof(*seed));
>                 } else {
>                         pr_err("Could not map UEFI random seed!\n");
> @@ -622,6 +622,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                         if (seed != NULL) {
>                                 pr_notice("seeding entropy pool\n");
>                                 add_bootloader_randomness(seed->bits, size);
> +                               memzero_explicit(seed->bits, size);
>                                 early_memunmap(seed, sizeof(*seed) + size);
>                         } else {
>                                 pr_err("Could not map UEFI random seed!\n");
> --
> 2.38.1
>