From: Ard Biesheuvel <email@example.com>
To: Eric Biggers <firstname.lastname@example.org>
Cc: Chris von Recklinghausen <email@example.com>,
Simo Sorce <firstname.lastname@example.org>,
"Rafael J. Wysocki" <email@example.com>,
Dexuan Cui <firstname.lastname@example.org>,
Linux PM <email@example.com>,
Linux Crypto Mailing List <firstname.lastname@example.org>,
Linux Kernel Mailing List <email@example.com>
Subject: Re: [PATCH v6 1/1] use crc32 instead of md5 for hibernation e820 integrity check
Date: Mon, 12 Apr 2021 21:27:59 +0200 [thread overview]
Message-ID: <CAMj1kXGZt8+5MVG-mNi67KsG8=4HCqEPs+RkrtzHusmCPFqSTg@mail.gmail.com> (raw)
On Mon, 12 Apr 2021 at 21:20, Eric Biggers <firstname.lastname@example.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
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.
next prev parent reply other threads:[~2021-04-12 19:28 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-12 14:09 [PATCH v6 1/1] use crc32 instead of md5 for hibernation e820 integrity check Chris von Recklinghausen
2021-04-12 17:45 ` Eric Biggers
2021-04-12 19:04 ` Chris von Recklinghausen
2021-04-12 19:20 ` Eric Biggers
2021-04-12 19:24 ` Chris von Recklinghausen
2021-04-12 19:27 ` Ard Biesheuvel [this message]
2021-04-12 19:51 ` Chris von Recklinghausen
2021-04-12 20:29 ` Ard Biesheuvel
2021-04-12 21:11 ` Simo Sorce
2021-04-13 9:09 ` David Laight
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).