Date:   Thu, 10 Nov 2022 10:32:06 -0800
From:   Evan Green <evgreen@...omium.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, corbet@....net,
        linux-pm@...r.kernel.org, rjw@...ysocki.net, gwendal@...omium.org,
        apronin@...omium.org, Pavel Machek <pavel@....cz>,
        Matthew Garrett <mgarrett@...ora.tech>,
        linux-integrity@...r.kernel.org, jejb@...ux.ibm.com,
        zohar@...ux.ibm.com, dlunev@...gle.com,
        Eric Biggers <ebiggers@...nel.org>,
        Ben Boeckel <me@...boeckel.net>, jarkko@...nel.org,
        Len Brown <len.brown@...el.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v4 09/11] PM: hibernate: Mix user key in encrypted hibernate

On Thu, Nov 10, 2022 at 8:17 AM Kees Cook <keescook@...omium.org> wrote:
>
> On Wed, Nov 09, 2022 at 04:30:10PM -0800, Evan Green wrote:
> > On Fri, Nov 4, 2022 at 11:54 AM Kees Cook <keescook@...omium.org> wrote:
> > >
> > > On Thu, Nov 03, 2022 at 11:01:17AM -0700, Evan Green wrote:
> > > > Usermode may have their own data protection requirements when it comes
> > > > to encrypting the hibernate image. For example, users may want a policy
> > > > where the hibernate image is protected by a key derived both from
> > > > platform-level security as well as authentication data (such as a
> > > > password or PIN). This way, even if the platform is compromised (ie a
> > > > stolen laptop), sensitive data cannot be exfiltrated via the hibernate
> > > > image without additional data (like the user's password).
> > > >
> > > > The kernel is already doing the encryption, but will be protecting its
> > > > key with the TPM alone. Allow usermode to mix in key content of their own
> > > > for the data portion of the hibernate image, so that the image
> > > > encryption key is determined both by a TPM-backed secret and
> > > > user-defined data.
> > > >
> > > > To mix the user key in, we hash the kernel key followed by the user key,
> > > > and use the resulting hash as the new key. This allows usermode to mix
> > > > in its key material without giving it too much control over what key is
> > > > actually driving the encryption (which might be used to attack the
> > > > secret kernel key).
> > > >
> > > > Limiting this to the data portion allows the kernel to receive the page
> > > > map and prepare its giant allocation even if this user key is not yet
> > > > available (ie the user has not yet finished typing in their password).
> > > > Once the user key becomes available, the data portion can be pushed
> > > > through to the kernel as well. This enables "preloading" scenarios,
> > > > where the hibernate image is loaded off of disk while the additional
> > > > key material (eg password) is being collected.
> > > >
> > > > One annoyance of the "preloading" scheme is that hibernate image memory
> > > > is effectively double-allocated: first by the usermode process pulling
> > > > encrypted contents off of disk and holding it, and second by the kernel
> > > > in its giant allocation in prepare_image(). An interesting future
> > > > optimization would be to allow the kernel to accept and store encrypted
> > > > page data before the user key is available. This would remove the
> > > > double allocation problem, as usermode could push the encrypted pages
> > > > loaded from disk immediately without storing them. The kernel could defer
> > > > decryption of the data until the user key is available, while still
> > > > knowing the correct page locations to store the encrypted data in.
> > > >
> > > > Signed-off-by: Evan Green <evgreen@...omium.org>
> > > > ---
> > > >
> > > > (no changes since v2)
> > > >
> > > > Changes in v2:
> > > >  - Add missing static on snapshot_encrypted_byte_count()
> > > >  - Fold in only the used kernel key bytes to the user key.
> > > >  - Make the user key length 32 (Eric)
> > > >  - Use CRYPTO_LIB_SHA256 for less boilerplate (Eric)
> > > >
> > > >  include/uapi/linux/suspend_ioctls.h |  15 ++-
> > > >  kernel/power/Kconfig                |   1 +
> > > >  kernel/power/power.h                |   1 +
> > > >  kernel/power/snapenc.c              | 158 ++++++++++++++++++++++++++--
> > > >  kernel/power/snapshot.c             |   5 +
> > > >  kernel/power/user.c                 |   4 +
> > > >  kernel/power/user.h                 |  12 +++
> > > >  7 files changed, 185 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/suspend_ioctls.h b/include/uapi/linux/suspend_ioctls.h
> > > > index b73026ef824bb9..f93a22eac52dc2 100644
> > > > --- a/include/uapi/linux/suspend_ioctls.h
> > > > +++ b/include/uapi/linux/suspend_ioctls.h
> > > > @@ -25,6 +25,18 @@ struct uswsusp_key_blob {
> > > >       __u8 nonce[USWSUSP_KEY_NONCE_SIZE];
> > > >  } __attribute__((packed));
> > > >
> > > > +/*
> > > > + * Allow user mode to fold in key material for the data portion of the hibernate
> > > > + * image.
> > > > + */
> > > > +struct uswsusp_user_key {
> > > > +     /* Kernel returns the metadata size. */
> > > > +     __kernel_loff_t meta_size;
> > > > +     __u32 key_len;
> > > > +     __u8 key[32];
> > >
> > > Why is this 32? (Is there a non-literal we can put here?)
> >
> > Sure, I can make a new define for this: USWSUSP_USER_KEY_SIZE. Really
> > it just needs to be enough key material that usermode feels like
> > they've swizzled things up enough. I wanted to avoid using a
> > particular implementation constant like AES_KEYSIZE_256 because I
> > wanted that to be a kernel implementation detail, and also wanted to
> > avoid adding additional header dependencies to suspend_ioctls.h.
>
> Can this just use __aligned(8) etc?

It's possible this is more an FFI issue that trails off the end of my
knowledge, so I should just drop the pad. But I'll dump out my
thoughts anyway for posterity:

My understanding is that the compiler pads the size of a struct up to
its required alignment so that arrays of the struct always stay
aligned. In this case, the sizeof() the struct both with and without
the pad member is 0x30, since __kernel_off_t has a required alignment
of 8. I had a couple of worries about that led me to naming that
padding:
 * Though this structure isn't copied out of the kernel today, I
didn't want some future change that did it to accidentally leak kernel
memory via the unnamed padding.

 * Given that the sizeof the struct is encoded into the ioctl number,
and we're to some extent relying on bespoke compiler behavior, I
thought the padding member might make us more resilient to an
unexpected compiler change later.

 * On the usermode side, there are a bunch of Rust rules that I don't
totally understand related to "soundness", undefined values (which the
padding between struct members is), and transmuting structs back and
forth to byte arrays.

I confirmed with someone smarter than me that I'm not running afoul of
the Rust rules by dropping the padding and dropping the __packed I had
in Rust definition of the struct, so I'll plan to drop the pad member
here in the next spin. A very long-winded "OK, will do" :)

-Evan

Powered by blists - more mailing lists