| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0hKbi1qmGtD4YAUBBkQX9FZTyb3KbityUkj4E1UsmW7Pw@mail.gmail.com>
Date: Wed, 21 Apr 2021 19:05:46 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Chris von Recklinghausen <crecklin@...hat.com>
Cc: Eric Biggers <ebiggers@...nel.org>,
Ard Biesheuvel <ardb@...nel.org>, Simo Sorce <simo@...hat.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Dexuan Cui <decui@...rosoft.com>,
Linux PM <linux-pm@...r.kernel.org>,
Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1 v10] x86/power use crc32 instead of md5 for
hibernation e820 integrity check
On Tue, Apr 20, 2021 at 3:00 PM Chris von Recklinghausen
<crecklin@...hat.com> wrote:
>
> Hibernation fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
>
> The check is intended to detect whether the E820 memory map provided
> by the firmware after cold boot unexpectedly differs from the one that
> was in use when the hibernation image was created. In this case, the
> hibernation image cannot be restored, as it may cover memory regions
> that are no longer available to the OS.
>
> A non-cryptographic checksum such as CRC-32 is sufficient to detect such
> inadvertent deviations.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map by md5 digest")
>
> Reviewed-by: Eric Biggers <ebiggers@...gle.com>
> Tested-by: Dexuan Cui <decui@...rosoft.com>
> Reviewed-by: Dexuan Cui <decui@...rosoft.com>
>
> Signed-off-by: Chris von Recklinghausen <crecklin@...hat.com>
> ---
> v1 -> v2
> bump up RESTORE_MAGIC
> v2 -> v3
> move embelishment from cover letter to commit comments (no code change)
> v3 -> v4
> add note to comments that md5 isn't used for encryption here.
> v4 -> v5
> reword comment per Simo's suggestion
> v5 -> v6
> use wording from Eric Biggers, use crc32_le instead of crc32 from crypto
> framework (crc32_le is in the core API and removes need for #defines)
> v6 -> v7
> reword with input from Eric/Ard/Simo, code changed per Eric's feedback
> v7 -> v8
> More feedback per Eric -
> change 'Suspend' to 'Hibernation' in commit comments, rename e820_digest to
> e820_checksum and change it to an unsigned long. rename get_e820_md5 to
> compute_e820_crc32 and have it return the checksum value instead of writing
> it into a user supplied buffer, get rid of hibernation_e820_save in favor of
> calling compute_e820_crc32 directly, likewise, get rid of
> hibernation_e820_mismatch in favor of comparing e820_checksum to the return
> value of compute_e820_crc32()
> v8 -> v9
> Make comment for compute_e820_crc32 more kerneldoc friendly. Also update
> comment about the e820 firmware table in arch/x86/kernel/e820.c since it
> also referred to MD5 and hibernation.
> v9 -> v10
> Don't line wrap Fixes: line, add Reviewed-by lines from Eric and Dexuan and
> Tested-by line from Dexuan. No code changed.
>
> arch/x86/kernel/e820.c | 4 +-
> arch/x86/power/hibernate.c | 89 ++++++--------------------------------
> 2 files changed, 16 insertions(+), 77 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 22aad412f965..629c4994f165 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -31,8 +31,8 @@
> * - inform the user about the firmware's notion of memory layout
> * via /sys/firmware/memmap
> *
> - * - the hibernation code uses it to generate a kernel-independent MD5
> - * fingerprint of the physical memory layout of a system.
> + * - the hibernation code uses it to generate a kernel-independent CRC32
> + * checksum of the physical memory layout of a system.
> *
> * - 'e820_table_kexec': a slightly modified (by the kernel) firmware version
> * passed to us by the bootloader - the major difference between
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..e94e0050a583 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -13,8 +13,8 @@
> #include <linux/kdebug.h>
> #include <linux/cpu.h>
> #include <linux/pgtable.h>
> -
> -#include <crypto/hash.h>
> +#include <linux/types.h>
> +#include <linux/crc32.h>
>
> #include <asm/e820/api.h>
> #include <asm/init.h>
> @@ -54,95 +54,33 @@ int pfn_is_nosave(unsigned long pfn)
> return pfn >= nosave_begin_pfn && pfn < nosave_end_pfn;
> }
>
> -
> -#define MD5_DIGEST_SIZE 16
> -
> struct restore_data_record {
> unsigned long jump_address;
> unsigned long jump_address_phys;
> unsigned long cr3;
> unsigned long magic;
> - u8 e820_digest[MD5_DIGEST_SIZE];
> + unsigned long e820_checksum;
> };
>
> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * compute_e820_crc32 - calculate crc32 of a given e820 table
> *
> * @table: the e820 table to be calculated
> - * @buf: the md5 result to be stored to
> + *
> + * Return: the resulting checksum
> */
> -static int get_e820_md5(struct e820_table *table, void *buf)
> +static inline u32 compute_e820_crc32(struct e820_table *table)
> {
> - struct crypto_shash *tfm;
> - struct shash_desc *desc;
> - int size;
> - int ret = 0;
> -
> - tfm = crypto_alloc_shash("md5", 0, 0);
> - if (IS_ERR(tfm))
> - return -ENOMEM;
> -
> - desc = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm),
> - GFP_KERNEL);
> - if (!desc) {
> - ret = -ENOMEM;
> - goto free_tfm;
> - }
> -
> - desc->tfm = tfm;
> -
> - size = offsetof(struct e820_table, entries) +
> + int size = offsetof(struct e820_table, entries) +
> sizeof(struct e820_entry) * table->nr_entries;
>
> - if (crypto_shash_digest(desc, (u8 *)table, size, buf))
> - ret = -EINVAL;
> -
> - kfree_sensitive(desc);
> -
> -free_tfm:
> - crypto_free_shash(tfm);
> - return ret;
> -}
> -
> -static int hibernation_e820_save(void *buf)
> -{
> - return get_e820_md5(e820_table_firmware, buf);
> -}
> -
> -static bool hibernation_e820_mismatch(void *buf)
> -{
> - int ret;
> - u8 result[MD5_DIGEST_SIZE];
> -
> - memset(result, 0, MD5_DIGEST_SIZE);
> - /* If there is no digest in suspend kernel, let it go. */
> - if (!memcmp(result, buf, MD5_DIGEST_SIZE))
> - return false;
> -
> - ret = get_e820_md5(e820_table_firmware, result);
> - if (ret)
> - return true;
> -
> - return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
> -}
> -#else
> -static int hibernation_e820_save(void *buf)
> -{
> - return 0;
> -}
> -
> -static bool hibernation_e820_mismatch(void *buf)
> -{
> - /* If md5 is not builtin for restore kernel, let it go. */
> - return false;
> + return ~crc32_le(~0, (unsigned char const *)table, size);
> }
> -#endif
>
> #ifdef CONFIG_X86_64
> -#define RESTORE_MAGIC 0x23456789ABCDEF01UL
> +#define RESTORE_MAGIC 0x23456789ABCDEF02UL
> #else
> -#define RESTORE_MAGIC 0x12345678UL
> +#define RESTORE_MAGIC 0x12345679UL
> #endif
>
> /**
> @@ -179,7 +117,8 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
> */
> rdr->cr3 = restore_cr3 & ~CR3_PCID_MASK;
>
> - return hibernation_e820_save(rdr->e820_digest);
> + rdr->e820_checksum = compute_e820_crc32(e820_table_firmware);
> + return 0;
> }
>
> /**
> @@ -200,7 +139,7 @@ int arch_hibernation_header_restore(void *addr)
> jump_address_phys = rdr->jump_address_phys;
> restore_cr3 = rdr->cr3;
>
> - if (hibernation_e820_mismatch(rdr->e820_digest)) {
> + if (rdr->e820_checksum != compute_e820_crc32(e820_table_firmware)) {
> pr_crit("Hibernate inconsistent memory map detected!\n");
> return -ENODEV;
> }
> --
Applied as 5.13 material under edited subject, thanks a lot to
everyone involved!
Powered by blists - more mailing lists