| 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: <76c0d185-8b97-f68c-9fbc-76e4c3020048@redhat.com>
Date: Wed, 21 Apr 2021 13:23:27 -0400
From: Chris von Recklinghausen <crecklin@...hat.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Eric Biggers <ebiggers@...nel.org>,
Ard Biesheuvel <ardb@...nel.org>, Simo Sorce <simo@...hat.com>,
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 4/21/21 1:05 PM, Rafael J. Wysocki wrote:
> 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!
>
...and thanks all for bearing with me on this.
Chris
Powered by blists - more mailing lists