Re: [PATCH v4 09/11] PM: hibernate: Mix user key in encrypted hibernate

[Kees Cook <keescook@chromium.org>]

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@chromium.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@chromium.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?

-- 
Kees Cook