Messages in this thread | | | Subject | Re: [PATCH 1/1 v10] x86/power use crc32 instead of md5 for hibernation e820 integrity check | From | Chris von Recklinghausen <> | Date | Wed, 21 Apr 2021 13:23:27 -0400 |
| |
On 4/21/21 1:05 PM, Rafael J. Wysocki wrote: > On Tue, Apr 20, 2021 at 3:00 PM Chris von Recklinghausen > <crecklin@redhat.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@google.com> >> Tested-by: Dexuan Cui <decui@microsoft.com> >> Reviewed-by: Dexuan Cui <decui@microsoft.com> >> >> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.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
| |