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

[PATCH v6 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.

This patch changes the integrity check algorithm from md5 to crc32.

The purpose of the integrity check is to detect possible differences
between the memory map used at the time when the hibernation image is
about to be loaded into memory and the memory map used at the image
creation time, because it is generally unsafe to load the image if the
current memory map doesn't match the one used when it was created. so
it is not intended as a cryptographic integrity check.

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

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)

 arch/x86/power/hibernate.c | 76 +++++++++++---------------------------
 1 file changed, 22 insertions(+), 54 deletions(-)

diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
index cd3914fc9f3d..f39e507e34ca 100644
--- a/arch/x86/power/hibernate.c
+++ b/arch/x86/power/hibernate.c
@@ -13,6 +13,8 @@
 #include <linux/kdebug.h>
 #include <linux/cpu.h>
 #include <linux/pgtable.h>
+#include <linux/types.h>
+#include <linux/crc32.h>
 
 #include <crypto/hash.h>
 
@@ -55,94 +57,60 @@ int pfn_is_nosave(unsigned long pfn)
 }
 
 
-#define MD5_DIGEST_SIZE 16
+#define CRC32_DIGEST_SIZE (sizeof (u32))
 
 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)
 /**
- * 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);
-	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;
+	int size = offsetof(struct e820_table, entries) +
+                sizeof(struct e820_entry) * table->nr_entries;
+	u32 ret;
 
-	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;
+	ret = crc32_le(0, (unsigned char const *)table, size);
+	memset(buf, 0, CRC32_DIGEST_SIZE);
+	memcpy(buf, (char *)&ret, sizeof (ret));
+	return 0;
 }
 
 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)
-{
-	return 0;
-}
-
-static bool hibernation_e820_mismatch(void *buf)
-{
-	/* If md5 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
 
 /**
-- 
2.27.0

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

[Eric Biggers]

On Mon, Apr 12, 2021 at 10:09:32AM -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.
> 
> This patch changes the integrity check algorithm from md5 to crc32.
> 
> The purpose of the integrity check is to detect possible differences
> between the memory map used at the time when the hibernation image is
> about to be loaded into memory and the memory map used at the image
> creation time, because it is generally unsafe to load the image if the
> current memory map doesn't match the one used when it was created. so
> it is not intended as a cryptographic integrity check.

This still doesn't actually explain why a non-cryptographic checksum is
sufficient.  "Detection of possible differences" could very well require
cryptographic authentication; it depends on whether malicious changes need to be
detected or not.

> 
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>        by md5 digest")
> 
> 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)
> 
>  arch/x86/power/hibernate.c | 76 +++++++++++---------------------------
>  1 file changed, 22 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index cd3914fc9f3d..f39e507e34ca 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -13,6 +13,8 @@
>  #include <linux/kdebug.h>
>  #include <linux/cpu.h>
>  #include <linux/pgtable.h>
> +#include <linux/types.h>
> +#include <linux/crc32.h>
>  
>  #include <crypto/hash.h>

crypto/hash.h is no longer needed.

>  
> @@ -55,94 +57,60 @@ int pfn_is_nosave(unsigned long pfn)
>  }
>  
>  
> -#define MD5_DIGEST_SIZE 16
> +#define CRC32_DIGEST_SIZE (sizeof (u32))
>  
>  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];
>  };

It would be simpler to just make this field 'unsigned long'.  Then there would
be no need to deal with memcpy().

> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
>  /**
> - * get_e820_md5 - calculate md5 according to given e820 table
> + * get_e820_crc32 - calculate crc32 according to given e820 table

Calling this "compute_e820_crc32()" would avoid confusion with retrieving the
previously-stored crc32 value.

> +	ret = crc32_le(0, (unsigned char const *)table, size);

It would be better to do ~crc32_le(~0, ...) (i.e., invert at beginning and end)
to match the recommended usage of CRC-32.  Unfortunately the library function
doesn't do the inversions by default.

>  static int hibernation_e820_save(void *buf)
>  {
> -	return get_e820_md5(e820_table_firmware, buf);
> +	return get_e820_crc32(e820_table_firmware, buf);
>  }

This should be inlined into arch_hibernation_header_save().  Also note that it
can no longer fail.

>  
>  static bool hibernation_e820_mismatch(void *buf)
>  {

This should be inlined into arch_hibernation_header_restore().

>  	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;

There's no need to handle the "no digest" case anymore, right?  Since crc32_le()
is always available.

- Eric

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

[Chris von Recklinghausen]

On 4/12/21 1:45 PM, Eric Biggers wrote:
> On Mon, Apr 12, 2021 at 10:09:32AM -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.
>>
>> This patch changes the integrity check algorithm from md5 to crc32.
>>
>> The purpose of the integrity check is to detect possible differences
>> between the memory map used at the time when the hibernation image is
>> about to be loaded into memory and the memory map used at the image
>> creation time, because it is generally unsafe to load the image if the
>> current memory map doesn't match the one used when it was created. so
>> it is not intended as a cryptographic integrity check.
> This still doesn't actually explain why a non-cryptographic checksum is
> sufficient.  "Detection of possible differences" could very well require
> cryptographic authentication; it depends on whether malicious changes need to be
> detected or not.

Hi Eric,

The cases that the commit comments for 62a03defeabd mention are the same 
as for this patch, e.g.

     1. Without this patch applied, it is possible that BIOS has
        provided an inconsistent memory map, but the resume kernel is still
        able to restore the image anyway(e.g, E820_RAM region is the 
superset
        of the previous one), although the system might be unstable. So this
        patch tries to treat any inconsistent e820 as illegal.

     2. Another case is, this patch replies on comparing the e820_saved, but
        currently the e820_save might not be strictly the same across
        hibernation, even if BIOS has provided consistent e820 map - In
        theory mptable might modify the BIOS-provided e820_saved dynamically
        in early_reserve_e820_mpc_new, which would allocate a buffer from
        E820_RAM, and marks it from E820_RAM to E820_RESERVED).
        This is a potential and rare case we need to deal with in OS in
        the future.

Maybe they should be added to the comments with this patch as well? In 
any case, the above comments only mention detecting consequences of BIOS 
issues/actions on the e820 map and not intrusions from attackers 
requiring cryptographic protection. Does that seem to be a reasonable 
explanation to you? If so I can add these to the commit comments.

I'll make the other changes you suggest below.

Thanks,

Chris

>
>> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>>         by md5 digest")
>>
>> 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)
>>
>>   arch/x86/power/hibernate.c | 76 +++++++++++---------------------------
>>   1 file changed, 22 insertions(+), 54 deletions(-)
>>
>> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
>> index cd3914fc9f3d..f39e507e34ca 100644
>> --- a/arch/x86/power/hibernate.c
>> +++ b/arch/x86/power/hibernate.c
>> @@ -13,6 +13,8 @@
>>   #include <linux/kdebug.h>
>>   #include <linux/cpu.h>
>>   #include <linux/pgtable.h>
>> +#include <linux/types.h>
>> +#include <linux/crc32.h>
>>   
>>   #include <crypto/hash.h>
> crypto/hash.h is no longer needed.
>
>>   
>> @@ -55,94 +57,60 @@ int pfn_is_nosave(unsigned long pfn)
>>   }
>>   
>>   
>> -#define MD5_DIGEST_SIZE 16
>> +#define CRC32_DIGEST_SIZE (sizeof (u32))
>>   
>>   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];
>>   };
> It would be simpler to just make this field 'unsigned long'.  Then there would
> be no need to deal with memcpy().
>
>> -#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
>>   /**
>> - * get_e820_md5 - calculate md5 according to given e820 table
>> + * get_e820_crc32 - calculate crc32 according to given e820 table
> Calling this "compute_e820_crc32()" would avoid confusion with retrieving the
> previously-stored crc32 value.
>
>> +	ret = crc32_le(0, (unsigned char const *)table, size);
> It would be better to do ~crc32_le(~0, ...) (i.e., invert at beginning and end)
> to match the recommended usage of CRC-32.  Unfortunately the library function
> doesn't do the inversions by default.
>
>>   static int hibernation_e820_save(void *buf)
>>   {
>> -	return get_e820_md5(e820_table_firmware, buf);
>> +	return get_e820_crc32(e820_table_firmware, buf);
>>   }
> This should be inlined into arch_hibernation_header_save().  Also note that it
> can no longer fail.
>
>>   
>>   static bool hibernation_e820_mismatch(void *buf)
>>   {
> This should be inlined into arch_hibernation_header_restore().
>
>>   	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;
> There's no need to handle the "no digest" case anymore, right?  Since crc32_le()
> is always available.
>
> - Eric
>

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

[Eric Biggers]

On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote:
> On 4/12/21 1:45 PM, Eric Biggers wrote:
> > On Mon, Apr 12, 2021 at 10:09:32AM -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.
> > > 
> > > This patch changes the integrity check algorithm from md5 to crc32.
> > > 
> > > The purpose of the integrity check is to detect possible differences
> > > between the memory map used at the time when the hibernation image is
> > > about to be loaded into memory and the memory map used at the image
> > > creation time, because it is generally unsafe to load the image if the
> > > current memory map doesn't match the one used when it was created. so
> > > it is not intended as a cryptographic integrity check.
> > This still doesn't actually explain why a non-cryptographic checksum is
> > sufficient.  "Detection of possible differences" could very well require
> > cryptographic authentication; it depends on whether malicious changes need to be
> > detected or not.
> 
> Hi Eric,
> 
> The cases that the commit comments for 62a03defeabd mention are the same as
> for this patch, e.g.
> 
>     1. Without this patch applied, it is possible that BIOS has
>        provided an inconsistent memory map, but the resume kernel is still
>        able to restore the image anyway(e.g, E820_RAM region is the superset
>        of the previous one), although the system might be unstable. So this
>        patch tries to treat any inconsistent e820 as illegal.
> 
>     2. Another case is, this patch replies on comparing the e820_saved, but
>        currently the e820_save might not be strictly the same across
>        hibernation, even if BIOS has provided consistent e820 map - In
>        theory mptable might modify the BIOS-provided e820_saved dynamically
>        in early_reserve_e820_mpc_new, which would allocate a buffer from
>        E820_RAM, and marks it from E820_RAM to E820_RESERVED).
>        This is a potential and rare case we need to deal with in OS in
>        the future.
> 
> Maybe they should be added to the comments with this patch as well? In any
> case, the above comments only mention detecting consequences of BIOS
> issues/actions on the e820 map and not intrusions from attackers requiring
> cryptographic protection. Does that seem to be a reasonable explanation to
> you? If so I can add these to the commit comments.
> 
> I'll make the other changes you suggest below.
> 
> Thanks,
> 

Those details are still missing the high-level point.  Is this just meant to
detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to
detect malicious changes?  That's all that really needs to be mentioned.

- Eric

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

[Chris von Recklinghausen]

On 4/12/21 3:20 PM, Eric Biggers wrote:
> On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote:
>> On 4/12/21 1:45 PM, Eric Biggers wrote:
>>> On Mon, Apr 12, 2021 at 10:09:32AM -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.
>>>>
>>>> This patch changes the integrity check algorithm from md5 to crc32.
>>>>
>>>> The purpose of the integrity check is to detect possible differences
>>>> between the memory map used at the time when the hibernation image is
>>>> about to be loaded into memory and the memory map used at the image
>>>> creation time, because it is generally unsafe to load the image if the
>>>> current memory map doesn't match the one used when it was created. so
>>>> it is not intended as a cryptographic integrity check.
>>> This still doesn't actually explain why a non-cryptographic checksum is
>>> sufficient.  "Detection of possible differences" could very well require
>>> cryptographic authentication; it depends on whether malicious changes need to be
>>> detected or not.
>> Hi Eric,
>>
>> The cases that the commit comments for 62a03defeabd mention are the same as
>> for this patch, e.g.
>>
>>      1. Without this patch applied, it is possible that BIOS has
>>         provided an inconsistent memory map, but the resume kernel is still
>>         able to restore the image anyway(e.g, E820_RAM region is the superset
>>         of the previous one), although the system might be unstable. So this
>>         patch tries to treat any inconsistent e820 as illegal.
>>
>>      2. Another case is, this patch replies on comparing the e820_saved, but
>>         currently the e820_save might not be strictly the same across
>>         hibernation, even if BIOS has provided consistent e820 map - In
>>         theory mptable might modify the BIOS-provided e820_saved dynamically
>>         in early_reserve_e820_mpc_new, which would allocate a buffer from
>>         E820_RAM, and marks it from E820_RAM to E820_RESERVED).
>>         This is a potential and rare case we need to deal with in OS in
>>         the future.
>>
>> Maybe they should be added to the comments with this patch as well? In any
>> case, the above comments only mention detecting consequences of BIOS
>> issues/actions on the e820 map and not intrusions from attackers requiring
>> cryptographic protection. Does that seem to be a reasonable explanation to
>> you? If so I can add these to the commit comments.
>>
>> I'll make the other changes you suggest below.
>>
>> Thanks,
>>
> Those details are still missing the high-level point.  Is this just meant to
> detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to
> detect malicious changes?  That's all that really needs to be mentioned.


Ok, I'll say the intent is to detect non-malicious changes presumably 
from BIOS issues.

Thanks,

Chris

>
> - Eric
>

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

[Ard Biesheuvel]

On Mon, 12 Apr 2021 at 21:20, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote:
> > On 4/12/21 1:45 PM, Eric Biggers wrote:
> > > On Mon, Apr 12, 2021 at 10:09:32AM -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.
> > > >
> > > > This patch changes the integrity check algorithm from md5 to crc32.
> > > >
> > > > The purpose of the integrity check is to detect possible differences
> > > > between the memory map used at the time when the hibernation image is
> > > > about to be loaded into memory and the memory map used at the image
> > > > creation time, because it is generally unsafe to load the image if the
> > > > current memory map doesn't match the one used when it was created. so
> > > > it is not intended as a cryptographic integrity check.
> > > This still doesn't actually explain why a non-cryptographic checksum is
> > > sufficient.  "Detection of possible differences" could very well require
> > > cryptographic authentication; it depends on whether malicious changes need to be
> > > detected or not.
> >
> > Hi Eric,
> >
> > The cases that the commit comments for 62a03defeabd mention are the same as
> > for this patch, e.g.
> >
> >     1. Without this patch applied, it is possible that BIOS has
> >        provided an inconsistent memory map, but the resume kernel is still
> >        able to restore the image anyway(e.g, E820_RAM region is the superset
> >        of the previous one), although the system might be unstable. So this
> >        patch tries to treat any inconsistent e820 as illegal.
> >
> >     2. Another case is, this patch replies on comparing the e820_saved, but
> >        currently the e820_save might not be strictly the same across
> >        hibernation, even if BIOS has provided consistent e820 map - In
> >        theory mptable might modify the BIOS-provided e820_saved dynamically
> >        in early_reserve_e820_mpc_new, which would allocate a buffer from
> >        E820_RAM, and marks it from E820_RAM to E820_RESERVED).
> >        This is a potential and rare case we need to deal with in OS in
> >        the future.
> >
> > Maybe they should be added to the comments with this patch as well? In any
> > case, the above comments only mention detecting consequences of BIOS
> > issues/actions on the e820 map and not intrusions from attackers requiring
> > cryptographic protection. Does that seem to be a reasonable explanation to
> > you? If so I can add these to the commit comments.
> >
> > I'll make the other changes you suggest below.
> >
> > Thanks,
> >
>
> Those details are still missing the high-level point.  Is this just meant to
> detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to
> detect malicious changes?  That's all that really needs to be mentioned.
>

This is not about BIOS bugs. Hibernation is deep suspend/resume
grafted onto cold boot, and it is perfectly legal for the firmware to
present a different memory map to the OS after a cold boot. It is
Linux that decides that it can restore the entire system state from a
swap file, and carry on as if the cold boot was just a [firmware
assisted] suspend/resume.

So forging collisions is *not* a concern here. Let's avoid accidental
or malicious, as those adjectives seem to confuse some people. The
bottom line is that there is no need to protect against deliberate
attempts to hide the fact that the memory map has changed, and so
there is no reason to use cryptographic hashes here.

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

[Chris von Recklinghausen]

On 4/12/21 3:27 PM, Ard Biesheuvel wrote:
> On Mon, 12 Apr 2021 at 21:20, Eric Biggers <ebiggers@kernel.org> wrote:
>> On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote:
>>> On 4/12/21 1:45 PM, Eric Biggers wrote:
>>>> On Mon, Apr 12, 2021 at 10:09:32AM -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.
>>>>>
>>>>> This patch changes the integrity check algorithm from md5 to crc32.
>>>>>
>>>>> The purpose of the integrity check is to detect possible differences
>>>>> between the memory map used at the time when the hibernation image is
>>>>> about to be loaded into memory and the memory map used at the image
>>>>> creation time, because it is generally unsafe to load the image if the
>>>>> current memory map doesn't match the one used when it was created. so
>>>>> it is not intended as a cryptographic integrity check.
>>>> This still doesn't actually explain why a non-cryptographic checksum is
>>>> sufficient.  "Detection of possible differences" could very well require
>>>> cryptographic authentication; it depends on whether malicious changes need to be
>>>> detected or not.
>>> Hi Eric,
>>>
>>> The cases that the commit comments for 62a03defeabd mention are the same as
>>> for this patch, e.g.
>>>
>>>      1. Without this patch applied, it is possible that BIOS has
>>>         provided an inconsistent memory map, but the resume kernel is still
>>>         able to restore the image anyway(e.g, E820_RAM region is the superset
>>>         of the previous one), although the system might be unstable. So this
>>>         patch tries to treat any inconsistent e820 as illegal.
>>>
>>>      2. Another case is, this patch replies on comparing the e820_saved, but
>>>         currently the e820_save might not be strictly the same across
>>>         hibernation, even if BIOS has provided consistent e820 map - In
>>>         theory mptable might modify the BIOS-provided e820_saved dynamically
>>>         in early_reserve_e820_mpc_new, which would allocate a buffer from
>>>         E820_RAM, and marks it from E820_RAM to E820_RESERVED).
>>>         This is a potential and rare case we need to deal with in OS in
>>>         the future.
>>>
>>> Maybe they should be added to the comments with this patch as well? In any
>>> case, the above comments only mention detecting consequences of BIOS
>>> issues/actions on the e820 map and not intrusions from attackers requiring
>>> cryptographic protection. Does that seem to be a reasonable explanation to
>>> you? If so I can add these to the commit comments.
>>>
>>> I'll make the other changes you suggest below.
>>>
>>> Thanks,
>>>
>> Those details are still missing the high-level point.  Is this just meant to
>> detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to
>> detect malicious changes?  That's all that really needs to be mentioned.
>>
> This is not about BIOS bugs. Hibernation is deep suspend/resume
> grafted onto cold boot, and it is perfectly legal for the firmware to
> present a different memory map to the OS after a cold boot. It is
> Linux that decides that it can restore the entire system state from a
> swap file, and carry on as if the cold boot was just a [firmware
> assisted] suspend/resume.
>
> So forging collisions is *not* a concern here. Let's avoid accidental
> or malicious, as those adjectives seem to confuse some people. The
> bottom line is that there is no need to protect against deliberate
> attempts to hide the fact that the memory map has changed, and so
> there is no reason to use cryptographic hashes here.
>
How about :

The check is intended to differentiate between a resume (which expects 
an identical e820 map to the one saved in suspend), and a cold boot 
(which need not have an identical e820 map to that saved in suspend if 
any was done at all). It is not necessary here to protect against 
deliberate attempts to hide the fact that the memory map has changed, so 
crc32 is sufficient for detection.

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

[Ard Biesheuvel]

On Mon, 12 Apr 2021 at 21:51, Chris von Recklinghausen
<crecklin@redhat.com> wrote:
>
> On 4/12/21 3:27 PM, Ard Biesheuvel wrote:
> > On Mon, 12 Apr 2021 at 21:20, Eric Biggers <ebiggers@kernel.org> wrote:
> >> On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote:
> >>> On 4/12/21 1:45 PM, Eric Biggers wrote:
> >>>> On Mon, Apr 12, 2021 at 10:09:32AM -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.
> >>>>>
> >>>>> This patch changes the integrity check algorithm from md5 to crc32.
> >>>>>
> >>>>> The purpose of the integrity check is to detect possible differences
> >>>>> between the memory map used at the time when the hibernation image is
> >>>>> about to be loaded into memory and the memory map used at the image
> >>>>> creation time, because it is generally unsafe to load the image if the
> >>>>> current memory map doesn't match the one used when it was created. so
> >>>>> it is not intended as a cryptographic integrity check.
> >>>> This still doesn't actually explain why a non-cryptographic checksum is
> >>>> sufficient.  "Detection of possible differences" could very well require
> >>>> cryptographic authentication; it depends on whether malicious changes need to be
> >>>> detected or not.
> >>> Hi Eric,
> >>>
> >>> The cases that the commit comments for 62a03defeabd mention are the same as
> >>> for this patch, e.g.
> >>>
> >>>      1. Without this patch applied, it is possible that BIOS has
> >>>         provided an inconsistent memory map, but the resume kernel is still
> >>>         able to restore the image anyway(e.g, E820_RAM region is the superset
> >>>         of the previous one), although the system might be unstable. So this
> >>>         patch tries to treat any inconsistent e820 as illegal.
> >>>
> >>>      2. Another case is, this patch replies on comparing the e820_saved, but
> >>>         currently the e820_save might not be strictly the same across
> >>>         hibernation, even if BIOS has provided consistent e820 map - In
> >>>         theory mptable might modify the BIOS-provided e820_saved dynamically
> >>>         in early_reserve_e820_mpc_new, which would allocate a buffer from
> >>>         E820_RAM, and marks it from E820_RAM to E820_RESERVED).
> >>>         This is a potential and rare case we need to deal with in OS in
> >>>         the future.
> >>>
> >>> Maybe they should be added to the comments with this patch as well? In any
> >>> case, the above comments only mention detecting consequences of BIOS
> >>> issues/actions on the e820 map and not intrusions from attackers requiring
> >>> cryptographic protection. Does that seem to be a reasonable explanation to
> >>> you? If so I can add these to the commit comments.
> >>>
> >>> I'll make the other changes you suggest below.
> >>>
> >>> Thanks,
> >>>
> >> Those details are still missing the high-level point.  Is this just meant to
> >> detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to
> >> detect malicious changes?  That's all that really needs to be mentioned.
> >>
> > This is not about BIOS bugs. Hibernation is deep suspend/resume
> > grafted onto cold boot, and it is perfectly legal for the firmware to
> > present a different memory map to the OS after a cold boot. It is
> > Linux that decides that it can restore the entire system state from a
> > swap file, and carry on as if the cold boot was just a [firmware
> > assisted] suspend/resume.
> >
> > So forging collisions is *not* a concern here. Let's avoid accidental
> > or malicious, as those adjectives seem to confuse some people. The
> > bottom line is that there is no need to protect against deliberate
> > attempts to hide the fact that the memory map has changed, and so
> > there is no reason to use cryptographic hashes here.
> >
> How about :
>
> The check is intended to differentiate between a resume (which expects
> an identical e820 map to the one saved in suspend), and a cold boot
> (which need not have an identical e820 map to that saved in suspend if
> any was done at all). It is not necessary here to protect against
> deliberate attempts to hide the fact that the memory map has changed, so
> crc32 is sufficient for detection.
>

Almost. Hibernation always occurs after a cold boot, but usually, the
E820 memory map happens to be the same.

How about

"""
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 hash such as CRC-32 is sufficient to detect such
inadvertent deviations.
"""

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

[Simo Sorce]

On Mon, 2021-04-12 at 22:29 +0200, Ard Biesheuvel wrote:
> On Mon, 12 Apr 2021 at 21:51, Chris von Recklinghausen
> <crecklin@redhat.com> wrote:
> > On 4/12/21 3:27 PM, Ard Biesheuvel wrote:
> > > On Mon, 12 Apr 2021 at 21:20, Eric Biggers <ebiggers@kernel.org> wrote:
> > > > On Mon, Apr 12, 2021 at 03:04:58PM -0400, Chris von Recklinghausen wrote:
> > > > > On 4/12/21 1:45 PM, Eric Biggers wrote:
> > > > > > On Mon, Apr 12, 2021 at 10:09:32AM -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.
> > > > > > > 
> > > > > > > This patch changes the integrity check algorithm from md5 to crc32.
> > > > > > > 
> > > > > > > The purpose of the integrity check is to detect possible differences
> > > > > > > between the memory map used at the time when the hibernation image is
> > > > > > > about to be loaded into memory and the memory map used at the image
> > > > > > > creation time, because it is generally unsafe to load the image if the
> > > > > > > current memory map doesn't match the one used when it was created. so
> > > > > > > it is not intended as a cryptographic integrity check.
> > > > > > This still doesn't actually explain why a non-cryptographic checksum is
> > > > > > sufficient.  "Detection of possible differences" could very well require
> > > > > > cryptographic authentication; it depends on whether malicious changes need to be
> > > > > > detected or not.
> > > > > Hi Eric,
> > > > > 
> > > > > The cases that the commit comments for 62a03defeabd mention are the same as
> > > > > for this patch, e.g.
> > > > > 
> > > > >      1. Without this patch applied, it is possible that BIOS has
> > > > >         provided an inconsistent memory map, but the resume kernel is still
> > > > >         able to restore the image anyway(e.g, E820_RAM region is the superset
> > > > >         of the previous one), although the system might be unstable. So this
> > > > >         patch tries to treat any inconsistent e820 as illegal.
> > > > > 
> > > > >      2. Another case is, this patch replies on comparing the e820_saved, but
> > > > >         currently the e820_save might not be strictly the same across
> > > > >         hibernation, even if BIOS has provided consistent e820 map - In
> > > > >         theory mptable might modify the BIOS-provided e820_saved dynamically
> > > > >         in early_reserve_e820_mpc_new, which would allocate a buffer from
> > > > >         E820_RAM, and marks it from E820_RAM to E820_RESERVED).
> > > > >         This is a potential and rare case we need to deal with in OS in
> > > > >         the future.
> > > > > 
> > > > > Maybe they should be added to the comments with this patch as well? In any
> > > > > case, the above comments only mention detecting consequences of BIOS
> > > > > issues/actions on the e820 map and not intrusions from attackers requiring
> > > > > cryptographic protection. Does that seem to be a reasonable explanation to
> > > > > you? If so I can add these to the commit comments.
> > > > > 
> > > > > I'll make the other changes you suggest below.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > Those details are still missing the high-level point.  Is this just meant to
> > > > detect non-malicious changes (presumably caused by BIOS bugs), or is it meant to
> > > > detect malicious changes?  That's all that really needs to be mentioned.
> > > > 
> > > This is not about BIOS bugs. Hibernation is deep suspend/resume
> > > grafted onto cold boot, and it is perfectly legal for the firmware to
> > > present a different memory map to the OS after a cold boot. It is
> > > Linux that decides that it can restore the entire system state from a
> > > swap file, and carry on as if the cold boot was just a [firmware
> > > assisted] suspend/resume.
> > > 
> > > So forging collisions is *not* a concern here. Let's avoid accidental
> > > or malicious, as those adjectives seem to confuse some people. The
> > > bottom line is that there is no need to protect against deliberate
> > > attempts to hide the fact that the memory map has changed, and so
> > > there is no reason to use cryptographic hashes here.
> > > 
> > How about :
> > 
> > The check is intended to differentiate between a resume (which expects
> > an identical e820 map to the one saved in suspend), and a cold boot
> > (which need not have an identical e820 map to that saved in suspend if
> > any was done at all). It is not necessary here to protect against
> > deliberate attempts to hide the fact that the memory map has changed, so
> > crc32 is sufficient for detection.
> > 
> 
> Almost. Hibernation always occurs after a cold boot, but usually, the
> E820 memory map happens to be the same.
> 
> How about
> 
> """
> 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 hash such as CRC-32 is sufficient to detect such
> inadvertent deviations.
> """

hash -> checksum

Simo.


-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

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

[David Laight]

From: Chris von Recklinghausen
> Sent: 12 April 2021 20:51
...
> > This is not about BIOS bugs. Hibernation is deep suspend/resume
> > grafted onto cold boot, and it is perfectly legal for the firmware to
> > present a different memory map to the OS after a cold boot. It is
> > Linux that decides that it can restore the entire system state from a
> > swap file, and carry on as if the cold boot was just a [firmware
> > assisted] suspend/resume.
> >
> > So forging collisions is *not* a concern here. Let's avoid accidental
> > or malicious, as those adjectives seem to confuse some people. The
> > bottom line is that there is no need to protect against deliberate
> > attempts to hide the fact that the memory map has changed, and so
> > there is no reason to use cryptographic hashes here.
> >
> How about :
> 
> The check is intended to differentiate between a resume (which expects
> an identical e820 map to the one saved in suspend), and a cold boot
> (which need not have an identical e820 map to that saved in suspend if
> any was done at all). It is not necessary here to protect against
> deliberate attempts to hide the fact that the memory map has changed, so
> crc32 is sufficient for detection.

That sort of implies that the 'resume' and 'cold boot' are
differenciated.

But the previous comment rather implies that it is the presence
of a valid saved image that signifies a 'resume'.

An interesting failure case would be moving the disk to a
different system.

	David

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