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 > <creck...@redhat.com> 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 <de...@microsoft.com> > > Reviewed-by: Dexuan Cui <de...@microsoft.com> > > Signed-off-by: Chris von Recklinghausen <creck...@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 > > > > 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