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. 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.
Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
by md5 digest")
Tested-by: Dexuan Cui <[email protected]>
Reviewed-by: Dexuan Cui <[email protected]>
Signed-off-by: Chris von Recklinghausen <[email protected]>
---
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
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
On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
<[email protected]> 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. This integrity check is used only to verify accidental
> corruption of the hybernation data
It isn't used for that.
In fact, it is used to detect differences between the memory map used
before hibernation and the one made available by the BIOS during the
subsequent resume. And the check is there, because it is generally
unsafe to load the hibernation image into memory if the current memory
map doesn't match the one used when the image was created.
> and is not intended as a cryptographic integrity check.
But this is true nevertheless, so I would write:
"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."
And please make the md5 spelling consistent.
> Md5 is overkill in this case and also disabled in FIPS mode because it
> is known to be broken for cryptographic purposes.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
> by md5 digest")
>
> Tested-by: Dexuan Cui <[email protected]>
> Reviewed-by: Dexuan Cui <[email protected]>
> Signed-off-by: Chris von Recklinghausen <[email protected]>
> ---
> 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
>
> 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
>
On Thu, 2021-04-08 at 15:32 +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> <[email protected]> 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. This integrity check is used only to verify accidental
> > corruption of the hybernation data
>
> It isn't used for that.
>
> In fact, it is used to detect differences between the memory map used
> before hibernation and the one made available by the BIOS during the
> subsequent resume. And the check is there, because it is generally
> unsafe to load the hibernation image into memory if the current memory
> map doesn't match the one used when the image was created.
>
> > and is not intended as a cryptographic integrity check.
>
> But this is true nevertheless, so I would write:
>
> "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."
>
> And please make the md5 spelling consistent.
This sounds much better thanks, feel free to add my Acked-by as well if
it is useful.
Simo.
> > Md5 is overkill in this case and also disabled in FIPS mode because it
> > is known to be broken for cryptographic purposes.
> >
> > Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
> > by md5 digest")
> >
> > Tested-by: Dexuan Cui <[email protected]>
> > Reviewed-by: Dexuan Cui <[email protected]>
> > Signed-off-by: Chris von Recklinghausen <[email protected]>
> > ---
> > 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
> >
> > 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
> >
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
On Thu, Apr 08, 2021 at 03:32:38PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> <[email protected]> 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. This integrity check is used only to verify accidental
> > corruption of the hybernation data
>
> It isn't used for that.
>
> In fact, it is used to detect differences between the memory map used
> before hibernation and the one made available by the BIOS during the
> subsequent resume. And the check is there, because it is generally
> unsafe to load the hibernation image into memory if the current memory
> map doesn't match the one used when the image was created.
So what types of "differences" are you trying to detect? If you need to detect
differences caused by someone who maliciously made changes ("malicious" implies
they may try to avoid detection), then you need to use a cryptographic hash
function (or a cryptographic MAC if the hash value isn't stored separately). If
you only need to detect non-malicious changes (normally these would be called
"accidental" changes, but sure, it could be changes that are "intentionally"
made provided that the other side can be trusted to not try to avoid
detection...), then a non-cryptographic checksum would be sufficient.
- Eric
On Thu, Apr 08, 2021 at 09:15:06AM -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. 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.
>
> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
> by md5 digest")
>
> Tested-by: Dexuan Cui <[email protected]>
> Reviewed-by: Dexuan Cui <[email protected]>
> Signed-off-by: Chris von Recklinghausen <[email protected]>
> ---
> 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
>
> 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)
Should CONFIG_CRYPTO_CRC32 be getting selected from somewhere?
If that is too hard because it would pull in too much of the crypto API, maybe
using the library interface to CRC-32 (lib/crc32.c) would be a better fit?
- Eric
On Thu, 2021-04-08 at 08:26 -0700, Eric Biggers wrote:
> On Thu, Apr 08, 2021 at 03:32:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> > <[email protected]> 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. This integrity check is used only to verify accidental
> > > corruption of the hybernation data
> >
> > It isn't used for that.
> >
> > In fact, it is used to detect differences between the memory map used
> > before hibernation and the one made available by the BIOS during the
> > subsequent resume. And the check is there, because it is generally
> > unsafe to load the hibernation image into memory if the current memory
> > map doesn't match the one used when the image was created.
>
> So what types of "differences" are you trying to detect? If you need to detect
> differences caused by someone who maliciously made changes ("malicious" implies
> they may try to avoid detection), then you need to use a cryptographic hash
> function (or a cryptographic MAC if the hash value isn't stored separately). If
> you only need to detect non-malicious changes (normally these would be called
> "accidental" changes, but sure, it could be changes that are "intentionally"
> made provided that the other side can be trusted to not try to avoid
> detection...), then a non-cryptographic checksum would be sufficient.
Wouldn't you also need a signature with a TPM key in that case?
An attacker that can change memory maps can also change the hash on
disk ? Unless the hash is in an encrypted partition I guess...
Simo.
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
On 4/8/21 11:30 AM, Eric Biggers wrote:
> On Thu, Apr 08, 2021 at 09:15:06AM -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. 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.
>>
>> Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
>> by md5 digest")
>>
>> Tested-by: Dexuan Cui <[email protected]>
>> Reviewed-by: Dexuan Cui <[email protected]>
>> Signed-off-by: Chris von Recklinghausen <[email protected]>
>> ---
>> 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
>>
>> 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)
> Should CONFIG_CRYPTO_CRC32 be getting selected from somewhere?
Yes, presumably from the same source that sets CONFIG_CRYPTO_MD5. Also
presumably there's value to not forcing the check if the config value is
not set.
>
> If that is too hard because it would pull in too much of the crypto API, maybe
> using the library interface to CRC-32 (lib/crc32.c) would be a better fit?
Based on my statement above, the intent is to provide a simple drop in
replacement for md5 so that users of FIPS mode can suspend/resume
without any errors.
Thanks,
Chris
>
> - Eric
>
On Thu, Apr 8, 2021 at 5:26 PM Eric Biggers <[email protected]> wrote:
>
> On Thu, Apr 08, 2021 at 03:32:38PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 3:15 PM Chris von Recklinghausen
> > <[email protected]> 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. This integrity check is used only to verify accidental
> > > corruption of the hybernation data
> >
> > It isn't used for that.
> >
> > In fact, it is used to detect differences between the memory map used
> > before hibernation and the one made available by the BIOS during the
> > subsequent resume. And the check is there, because it is generally
> > unsafe to load the hibernation image into memory if the current memory
> > map doesn't match the one used when the image was created.
>
> So what types of "differences" are you trying to detect? If you need to detect
> differences caused by someone who maliciously made changes ("malicious" implies
> they may try to avoid detection), then you need to use a cryptographic hash
> function (or a cryptographic MAC if the hash value isn't stored separately). If
> you only need to detect non-malicious changes (normally these would be called
> "accidental" changes, but sure, it could be changes that are "intentionally"
> made provided that the other side can be trusted to not try to avoid
> detection...)
That's the case here.
> then a non-cryptographic checksum would be sufficient.
On Thu, Apr 08, 2021 at 11:53:59AM -0400, Chris von Recklinghausen wrote:
> On 4/8/21 11:30 AM, Eric Biggers wrote:
> > On Thu, Apr 08, 2021 at 09:15:06AM -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. 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.
> > >
> > > Fixes: 62a03defeabd ("PM / hibernate: Verify the consistent of e820 memory map
> > > by md5 digest")
> > >
> > > Tested-by: Dexuan Cui <[email protected]>
> > > Reviewed-by: Dexuan Cui <[email protected]>
> > > Signed-off-by: Chris von Recklinghausen <[email protected]>
> > > ---
> > > 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
> > >
> > > 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)
> > Should CONFIG_CRYPTO_CRC32 be getting selected from somewhere?
>
>
> Yes, presumably from the same source that sets CONFIG_CRYPTO_MD5. Also
> presumably there's value to not forcing the check if the config value is not
> set.
I wouldn't be so sure about that. It might just be a bug that CONFIG_CRYPTO_MD5
wasn't being selected. Where is it documented that the user needed to set
CONFIG_CRYPTO_MD5=y if they wanted the hibernation image checksumming to work?
> >
> > If that is too hard because it would pull in too much of the crypto API, maybe
> > using the library interface to CRC-32 (lib/crc32.c) would be a better fit?
>
>
> Based on my statement above, the intent is to provide a simple drop in
> replacement for md5 so that users of FIPS mode can suspend/resume without
> any errors.
>
It's possible that most people have CONFIG_CRYPTO_MD5 enabled for some unrelated
reason so the hibernation image checksumming works by chance.
CONFIG_CRYPTO_CRC32 is a different option so the same is not necessarily true.
However, note that CONFIG_HIBERNATION already selects CONFIG_CRC32 (the library
interface to CRC-32) but not CONFIG_CRYPTO_CRC32 (shash interface to CRC-32).
So, if this code just used the library interface crc32_le(), it will always be
available and the IS_BUILTIN() checks can be removed...
- Eric
Hi!
> > > > 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. This integrity check is used only to verify accidental
> > > > corruption of the hybernation data
> > >
> > > It isn't used for that.
> > >
> > > In fact, it is used to detect differences between the memory map used
> > > before hibernation and the one made available by the BIOS during the
> > > subsequent resume. And the check is there, because it is generally
> > > unsafe to load the hibernation image into memory if the current memory
> > > map doesn't match the one used when the image was created.
> >
> > So what types of "differences" are you trying to detect? If you need to detect
> > differences caused by someone who maliciously made changes ("malicious" implies
> > they may try to avoid detection), then you need to use a cryptographic hash
> > function (or a cryptographic MAC if the hash value isn't stored separately). If
> > you only need to detect non-malicious changes (normally these would be called
> > "accidental" changes, but sure, it could be changes that are "intentionally"
> > made provided that the other side can be trusted to not try to avoid
> > detection...)
>
> That's the case here.
md5 is fine for this purpose. crc32 may be too weak. I don't see why this needs changing.
Maybe fips should understand that md5 has other uses than crypto?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Apr 29, 2021 at 09:59:44PM +0200, Pavel Machek wrote:
> Hi!
>
> > > > > 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. This integrity check is used only to verify accidental
> > > > > corruption of the hybernation data
> > > >
> > > > It isn't used for that.
> > > >
> > > > In fact, it is used to detect differences between the memory map used
> > > > before hibernation and the one made available by the BIOS during the
> > > > subsequent resume. And the check is there, because it is generally
> > > > unsafe to load the hibernation image into memory if the current memory
> > > > map doesn't match the one used when the image was created.
> > >
> > > So what types of "differences" are you trying to detect? If you need to detect
> > > differences caused by someone who maliciously made changes ("malicious" implies
> > > they may try to avoid detection), then you need to use a cryptographic hash
> > > function (or a cryptographic MAC if the hash value isn't stored separately). If
> > > you only need to detect non-malicious changes (normally these would be called
> > > "accidental" changes, but sure, it could be changes that are "intentionally"
> > > made provided that the other side can be trusted to not try to avoid
> > > detection...)
> >
> > That's the case here.
>
> md5 is fine for this purpose. crc32 may be too weak. I don't see why this needs changing.
>
> Maybe fips should understand that md5 has other uses than crypto?
>
> Best regards,
> Pavel
This would be a good change even if FIPS didn't exist, because either you need a
cryptographic digest or you don't. Using MD5 is a big red flag as there isn't
really any valid use case for it anymore. We should be working to eliminate all
uses of MD5 from the kernel, and likewise for other broken crypto algorithms
like RC4. Note that that includes not just upgrading crypto algorithms, but
also eliminating cases where crypto was improperly used when it isn't needed.
As far as non-cryptographic checksums go, CRC-32 has less than a 1 in 4 billion
chance of a collision. People seemed happy with that for this use case. But if
a stronger checksum is desired, then CRC-64 or xxHash64 would give a 1 in 2^64
chance of collision instead.
- Eric