[PATCH v4 1/1] use crc32 instead of md5 for hibernation e820 integrity check

[PATCH v4 1/1] use crc32 instead of md5 for hibernation e820 integrity check

[Chris von Recklinghausen]

Suspend fails on a system in fips mode because md5 is used for the e820
integrity check and is not available. Use crc32 instead.

Prior to this patch, MD5 is used only to create a digest to ensure integrity of
the region, no actual encryption is done. This patch set changes the integrity
check to use crc32 instead of md5 since crc32 is available in both FIPS and
non-FIPS modes.

Note that the digest is only used as an integrity check. No actual encryption
is done.

Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
       by md5 digest")

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.

 arch/x86/power/hibernate.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
index cd3914fc9f3d..b56172553275 100644
--- a/arch/x86/power/hibernate.c
+++ b/arch/x86/power/hibernate.c
@@ -55,31 +55,31 @@ int pfn_is_nosave(unsigned long pfn)
 }
 
 
-#define MD5_DIGEST_SIZE 16
+#define CRC32_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];
+	u8 e820_digest[CRC32_DIGEST_SIZE];
 };
 
-#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
+#if IS_BUILTIN(CONFIG_CRYPTO_CRC32)
 /**
- * get_e820_md5 - calculate md5 according to given e820 table
+ * get_e820_crc32 - calculate crc32 according to given e820 table
  *
  * @table: the e820 table to be calculated
- * @buf: the md5 result to be stored to
+ * @buf: the crc32 result to be stored to
  */
-static int get_e820_md5(struct e820_table *table, void *buf)
+static int get_e820_crc32(struct e820_table *table, void *buf)
 {
 	struct crypto_shash *tfm;
 	struct shash_desc *desc;
 	int size;
 	int ret = 0;
 
-	tfm = crypto_alloc_shash("md5", 0, 0);
+	tfm = crypto_alloc_shash("crc32", 0, 0);
 	if (IS_ERR(tfm))
 		return -ENOMEM;
 
@@ -107,24 +107,24 @@ static int get_e820_md5(struct e820_table *table, void *buf)
 
 static int hibernation_e820_save(void *buf)
 {
-	return get_e820_md5(e820_table_firmware, buf);
+	return get_e820_crc32(e820_table_firmware, buf);
 }
 
 static bool hibernation_e820_mismatch(void *buf)
 {
 	int ret;
-	u8 result[MD5_DIGEST_SIZE];
+	u8 result[CRC32_DIGEST_SIZE];
 
-	memset(result, 0, MD5_DIGEST_SIZE);
+	memset(result, 0, CRC32_DIGEST_SIZE);
 	/* If there is no digest in suspend kernel, let it go. */
-	if (!memcmp(result, buf, MD5_DIGEST_SIZE))
+	if (!memcmp(result, buf, CRC32_DIGEST_SIZE))
 		return false;
 
-	ret = get_e820_md5(e820_table_firmware, result);
+	ret = get_e820_crc32(e820_table_firmware, result);
 	if (ret)
 		return true;
 
-	return memcmp(result, buf, MD5_DIGEST_SIZE) ? true : false;
+	return memcmp(result, buf, CRC32_DIGEST_SIZE) ? true : false;
 }
 #else
 static int hibernation_e820_save(void *buf)
@@ -134,15 +134,15 @@ static int hibernation_e820_save(void *buf)
 
 static bool hibernation_e820_mismatch(void *buf)
 {
-	/* If md5 is not builtin for restore kernel, let it go. */
+	/* If crc32 is not builtin for restore kernel, let it go. */
 	return false;
 }
 #endif
 
 #ifdef CONFIG_X86_64
-#define RESTORE_MAGIC	0x23456789ABCDEF01UL
+#define RESTORE_MAGIC	0x23456789ABCDEF02UL
 #else
-#define RESTORE_MAGIC	0x12345678UL
+#define RESTORE_MAGIC	0x12345679UL
 #endif
 
 /**
@@ -160,6 +160,9 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
 	rdr->jump_address = (unsigned long)restore_registers;
 	rdr->jump_address_phys = __pa_symbol(restore_registers);
 
+	/* crc32 digest size is 4 but digest buffer size is 16 so zero it all */
+	memset(rdr->e820_digest, 0, CRC32_DIGEST_SIZE);
+
 	/*
 	 * The restore code fixes up CR3 and CR4 in the following sequence:
 	 *
-- 
2.18.1

Re: [PATCH v4 1/1] use crc32 instead of md5 for hibernation e820 integrity check

[Simo Sorce]

On Thu, 2021-04-08 at 06:46 -0400, Chris von Recklinghausen wrote:
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
> 
> Prior to this patch, MD5 is used only to create a digest to ensure integrity of
> the region, no actual encryption is done. This patch set changes the integrity
> check to use crc32 instead of md5 since crc32 is available in both FIPS and
> non-FIPS modes.
> 
> Note that the digest is only used as an integrity check. No actual encryption
> is done.
> 
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>        by md5 digest")
> 
> Tested-by: Dexuan Cui <decui@microsoft.com>
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>

Hi Chris,
I would reword it this way:
----------
This patch(set) changes the integrity check algorithm from md5 to
crc32. This integrity check is used only to verify accidental
corruption of the hybernation data and is not intended as a
cryptographic integrity check.
Md5 is overkill in this case and also disabled in FIPS mode because it
is known to be broken for cryptographic purposes.
----------

HTH,
Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

RE: [PATCH v4 1/1] use crc32 instead of md5 for hibernation e820 integrity check

[David Laight]

From: Chris von Recklinghausen
> Sent: 08 April 2021 11:46
> 
> Suspend fails on a system in fips mode because md5 is used for the e820
> integrity check and is not available. Use crc32 instead.
> 
> Prior to this patch, MD5 is used only to create a digest to ensure integrity of
> the region, no actual encryption is done. This patch set changes the integrity
> check to use crc32 instead of md5 since crc32 is available in both FIPS and
> non-FIPS modes.
> 
> Note that the digest is only used as an integrity check. No actual encryption
> is done.

If crc32 is good enough, would a 1's compliment sum be good enough?
It is likely to be faster to calculate and not need special
functions be built into the kernel at all.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v4 1/1] use crc32 instead of md5 for hibernation e820 integrity check

[Chris von Recklinghausen]

On 4/9/21 12:56 PM, David Laight wrote:
> From: Chris von Recklinghausen
>> Sent: 08 April 2021 11:46
>>
>> Suspend fails on a system in fips mode because md5 is used for the e820
>> integrity check and is not available. Use crc32 instead.
>>
>> Prior to this patch, MD5 is used only to create a digest to ensure integrity of
>> the region, no actual encryption is done. This patch set changes the integrity
>> check to use crc32 instead of md5 since crc32 is available in both FIPS and
>> non-FIPS modes.
>>
>> Note that the digest is only used as an integrity check. No actual encryption
>> is done.
> If crc32 is good enough, would a 1's compliment sum be good enough?
> It is likely to be faster to calculate and not need special
> functions be built into the kernel at all.


Eric Biggers <ebiggers@kernel.org> suggested using crc32_le() which is 
in the library interface (lib/crc32.c) and will always be available 
without any special ifdefs. That's what my next version will be based on.

Thanks,

Chris


>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>