| Message ID | 20210420125739.29353-1-crecklin@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1,v10] x86/power use crc32 instead of md5 for hibernation e820 integrity check | expand |
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!
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
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; }
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(-)