Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 4 May 2021 20:45:41 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Pavel Machek <pavel@ucw.cz>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Chris von Recklinghausen <crecklin@redhat.com>,
        Ard Biesheuvel <ardb@kernel.org>, Simo Sorce <simo@redhat.com>,
        Dexuan Cui <decui@microsoft.com>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 1/1] use crc32 instead of md5 for hibernation e820
 integrity check
Message-ID: <YJIU5fd5e2FwrEB/@gmail.com>
References: <20210408131506.17941-1-crecklin@redhat.com>
 <CAJZ5v0ib+jmbsD9taGW0RujY5c9BCK8yLHv065u44mb0AwO9vQ@mail.gmail.com>
 <YG8gqZoZGutPmROz@sol.localdomain>
 <CAJZ5v0g65irXKmy7pdgD8-5KWrxdtwiWbJsBD2A=PKf1D3RVZg@mail.gmail.com>
 <20210429195944.GB1067@bug>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210429195944.GB1067@bug>
Precedence: bulk

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