[PATCH 0/6] fscrypt: key verification and KDF improvement

[PATCH 0/6] fscrypt: key verification and KDF improvement

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This patch series solves two major problems which filesystem-level
encryption has currently.  First, the user-supplied master keys are not
verified, which means a malicious user can provide the wrong key for
another user's file and cause a DOS attack or worse.  This flaw has been
criticized in the past [1].  Second, the KDF (Key Derivation Function)
used to derive per-file keys is ad-hoc and nonstandard.  While it meets
the primary security requirement, it's inflexible and is missing some
useful properties such as non-reversibility, which is important under
some threat models.  This weakness was noted by Unterluggauer and
Mangard (2016) [2] who also demonstrated an EM attack against the
current AES-based KDF.

These problems are solved together by introducing a new encryption
policy version where the KDF is changed to HKDF-SHA512, i.e. RFC-5869
[3] with SHA-512 as the underlying hash function.  HKDF is used to
derive the per-file keys as well as to generate a "key hash" which is
stored on-disk to allow key verification.  The HMAC transform for each
master key is pre-keyed and cached, which in practice makes the new KDF
about as fast or even faster than the old one which did not use the
crypto API efficiently.

Please give special consideration to the choice and usage of crypto
algorithms and any other on-disk format and API changes, since we will
be locked into these once merged.

All these changes are independent of filesystem and encryption mode,
i.e. the "v2" encryption policies can be used on any fscrypt-capable
filesystem (ext4, f2fs, or ubifs currently) and with any of the
supported encryption modes.

References:
  [1] https://blog.quarkslab.com/a-glimpse-of-ext4-filesystem-level-encryption.html

  [2] Unterluggauer and Mangard (2016).  "Exploiting the Physical
      Disparity: Side-Channel Attacks on Memory Encryption".
      https://eprint.iacr.org/2016/473.pdf

  [3] RFC 5869. "HMAC-based Extract-and-Expand Key Derivation Function
      (HKDF)".  https://tools.ietf.org/html/rfc5869

Eric Biggers (6):
  fscrypt: add v2 encryption context and policy
  fscrypt: rename ->ci_master_key to ->ci_master_key_descriptor
  fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys
  fscrypt: verify that the correct master key was supplied
  fscrypt: cache the HMAC transform for each master key
  fscrypt: for v2 policies, support "fscrypt:" key prefix only

 fs/crypto/Kconfig              |   2 +
 fs/crypto/fscrypt_private.h    | 109 ++++++-
 fs/crypto/keyinfo.c            | 669 ++++++++++++++++++++++++++++++++++-------
 fs/crypto/policy.c             | 118 ++++++--
 fs/super.c                     |   4 +
 include/linux/fs.h             |   5 +
 include/linux/fscrypt_common.h |   2 +-
 include/uapi/linux/fs.h        |   6 +
 8 files changed, 766 insertions(+), 149 deletions(-)

-- 
2.13.2.932.g7449e964c-goog

[PATCH 1/6] fscrypt: add v2 encryption context and policy

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, the fscrypt_context (i.e. the encryption xattr) does not
contain a cryptographically secure identifier for the master key's
payload.  Therefore it's not possible to verify that the correct key was
supplied, which is problematic in multi-user scenarios.  To make this
possible, define a new fscrypt_context version (v2) which includes a
key_hash field, and allow userspace to opt-in to it when setting an
encryption policy by setting fscrypt_policy.version to 2.  For now just
zero the new field; a later patch will start setting it for real.

Even though we aren't changing the layout of struct fscrypt_policy (i.e.
the struct used by the ioctls), the new context version still has to be
"opt-in" because old kernels will not recognize it, and the keyring key
will now need to be available when setting an encryption policy, which
is an API change.  We'll also be taking the opportunity to make another
API change (dropping support for the filesystem-specific key prefixes).

Previously, the version numbers were 0 in the fscrypt_policy and 1 in
the fscrypt_context.  Rather than incrementing them to 1 and 2, make
them both 2 to be consistent with each other.  It's not required that
these numbers match, but it should make things less confusing.

An alternative to adding a key_hash field would have been to reuse
master_key_descriptor.  However, master_key_descriptor is only 8 bytes,
which is too short to be a cryptographically secure hash.  Thus,
master_key_descriptor would have needed to be lengthened to 16+ bytes,
which would have required defining a fscrypt_policy_v2 structure and
adding a FS_IOC_GET_ENCRYPTION_POLICY_V2 ioctl.  It also would have
required userspace to start using a specific hash algorithm to create
the key descriptors, which would have made the API harder to use.
Perhaps it should have been done that way originally, but at this point
it seems better to keep the API simpler.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h    | 79 ++++++++++++++++++++++++++++++++++--------
 fs/crypto/keyinfo.c            | 14 ++++----
 fs/crypto/policy.c             | 67 ++++++++++++++++++++++++++---------
 include/linux/fscrypt_common.h |  2 +-
 include/uapi/linux/fs.h        |  6 ++++
 5 files changed, 127 insertions(+), 41 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index a1d5021c31ef..ef6909035823 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -25,39 +25,88 @@
 #define FS_AES_256_XTS_KEY_SIZE		64
 
 #define FS_KEY_DERIVATION_NONCE_SIZE		16
+#define FSCRYPT_KEY_HASH_SIZE		16
 
 /**
- * Encryption context for inode
+ * fscrypt_context - the encryption context for an inode
  *
- * Protector format:
- *  1 byte: Protector format (1 = this version)
- *  1 byte: File contents encryption mode
- *  1 byte: File names encryption mode
- *  1 byte: Flags
- *  8 bytes: Master Key descriptor
- *  16 bytes: Encryption Key derivation nonce
+ * Filesystems usually store this in an extended attribute.  It identifies the
+ * encryption algorithm and key with which the file is encrypted.
  */
 struct fscrypt_context {
-	u8 format;
+	/* v1+ */
+
+	/* Version of this structure */
+	u8 version;
+
+	/* Encryption mode for the contents of regular files */
 	u8 contents_encryption_mode;
+
+	/* Encryption mode for filenames in directories and symlink targets */
 	u8 filenames_encryption_mode;
+
+	/* Options that affect how encryption is done (e.g. padding amount) */
 	u8 flags;
+
+	/* Descriptor for this file's master key in the keyring */
 	u8 master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
+
+	/*
+	 * A unique value used in combination with the master key to derive the
+	 * file's actual encryption key
+	 */
 	u8 nonce[FS_KEY_DERIVATION_NONCE_SIZE];
-} __packed;
 
-#define FS_ENCRYPTION_CONTEXT_FORMAT_V1		1
+	/* v2+ */
+
+	/* Cryptographically secure hash of the master key */
+	u8 key_hash[FSCRYPT_KEY_HASH_SIZE];
+};
+
+#define FSCRYPT_CONTEXT_V1	1
+#define FSCRYPT_CONTEXT_V1_SIZE	offsetof(struct fscrypt_context, key_hash)
+
+#define FSCRYPT_CONTEXT_V2	2
+#define FSCRYPT_CONTEXT_V2_SIZE	sizeof(struct fscrypt_context)
+
+static inline int fscrypt_context_size(const struct fscrypt_context *ctx)
+{
+	switch (ctx->version) {
+	case FSCRYPT_CONTEXT_V1:
+		return FSCRYPT_CONTEXT_V1_SIZE;
+	case FSCRYPT_CONTEXT_V2:
+		return FSCRYPT_CONTEXT_V2_SIZE;
+	}
+	return 0;
+}
+
+static inline bool
+fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
+{
+	return size >= 1 && size == fscrypt_context_size(ctx);
+}
 
 /*
- * A pointer to this structure is stored in the file system's in-core
- * representation of an inode.
+ * fscrypt_info - the "encryption key" for an inode
+ *
+ * When an encrypted file's key is made available, an instance of this struct is
+ * allocated and stored in ->i_crypt_info.  Once created, it remains until the
+ * inode is evicted.
  */
 struct fscrypt_info {
+
+	/* The actual crypto transforms needed for encryption and decryption */
+	struct crypto_skcipher *ci_ctfm;
+	struct crypto_cipher *ci_essiv_tfm;
+
+	/*
+	 * Cached fields from the fscrypt_context needed for encryption policy
+	 * inheritance and enforcement
+	 */
+	u8 ci_context_version;
 	u8 ci_data_mode;
 	u8 ci_filename_mode;
 	u8 ci_flags;
-	struct crypto_skcipher *ci_ctfm;
-	struct crypto_cipher *ci_essiv_tfm;
 	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
 };
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 018c588c7ac3..7e664a11340a 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -272,29 +272,27 @@ int fscrypt_get_encryption_info(struct inode *inode)
 			return res;
 		/* Fake up a context for an unencrypted directory */
 		memset(&ctx, 0, sizeof(ctx));
-		ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1;
+		ctx.version = FSCRYPT_CONTEXT_V1;
 		ctx.contents_encryption_mode = FS_ENCRYPTION_MODE_AES_256_XTS;
 		ctx.filenames_encryption_mode = FS_ENCRYPTION_MODE_AES_256_CTS;
 		memset(ctx.master_key_descriptor, 0x42, FS_KEY_DESCRIPTOR_SIZE);
-	} else if (res != sizeof(ctx)) {
-		return -EINVAL;
+		res = FSCRYPT_CONTEXT_V1_SIZE;
 	}
 
-	if (ctx.format != FS_ENCRYPTION_CONTEXT_FORMAT_V1)
+	if (!fscrypt_valid_context_format(&ctx, res))
 		return -EINVAL;
 
 	if (ctx.flags & ~FS_POLICY_FLAGS_VALID)
 		return -EINVAL;
 
-	crypt_info = kmem_cache_alloc(fscrypt_info_cachep, GFP_NOFS);
+	crypt_info = kmem_cache_zalloc(fscrypt_info_cachep, GFP_NOFS);
 	if (!crypt_info)
 		return -ENOMEM;
 
-	crypt_info->ci_flags = ctx.flags;
+	crypt_info->ci_context_version = ctx.version;
 	crypt_info->ci_data_mode = ctx.contents_encryption_mode;
 	crypt_info->ci_filename_mode = ctx.filenames_encryption_mode;
-	crypt_info->ci_ctfm = NULL;
-	crypt_info->ci_essiv_tfm = NULL;
+	crypt_info->ci_flags = ctx.flags;
 	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
 				sizeof(crypt_info->ci_master_key));
 
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index ce07a86200f3..044f23fadb5a 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -13,6 +13,28 @@
 #include <linux/mount.h>
 #include "fscrypt_private.h"
 
+static u8 policy_version_for_context(const struct fscrypt_context *ctx)
+{
+	switch (ctx->version) {
+	case FSCRYPT_CONTEXT_V1:
+		return FS_POLICY_VERSION_ORIGINAL;
+	case FSCRYPT_CONTEXT_V2:
+		return FS_POLICY_VERSION_HKDF;
+	}
+	BUG();
+}
+
+static u8 context_version_for_policy(const struct fscrypt_policy *policy)
+{
+	switch (policy->version) {
+	case FS_POLICY_VERSION_ORIGINAL:
+		return FSCRYPT_CONTEXT_V1;
+	case FS_POLICY_VERSION_HKDF:
+		return FSCRYPT_CONTEXT_V2;
+	}
+	BUG();
+}
+
 /*
  * check whether an encryption policy is consistent with an encryption context
  */
@@ -20,8 +42,10 @@ static bool is_encryption_context_consistent_with_policy(
 				const struct fscrypt_context *ctx,
 				const struct fscrypt_policy *policy)
 {
-	return memcmp(ctx->master_key_descriptor, policy->master_key_descriptor,
-		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+	return (ctx->version == context_version_for_policy(policy)) &&
+		(memcmp(ctx->master_key_descriptor,
+			policy->master_key_descriptor,
+			FS_KEY_DESCRIPTOR_SIZE) == 0) &&
 		(ctx->flags == policy->flags) &&
 		(ctx->contents_encryption_mode ==
 		 policy->contents_encryption_mode) &&
@@ -34,10 +58,6 @@ static int create_encryption_context_from_policy(struct inode *inode,
 {
 	struct fscrypt_context ctx;
 
-	ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1;
-	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
-					FS_KEY_DESCRIPTOR_SIZE);
-
 	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
 				     policy->filenames_encryption_mode))
 		return -EINVAL;
@@ -45,13 +65,20 @@ static int create_encryption_context_from_policy(struct inode *inode,
 	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
 		return -EINVAL;
 
+	ctx.version = context_version_for_policy(policy);
 	ctx.contents_encryption_mode = policy->contents_encryption_mode;
 	ctx.filenames_encryption_mode = policy->filenames_encryption_mode;
 	ctx.flags = policy->flags;
+	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
+	       FS_KEY_DESCRIPTOR_SIZE);
 	BUILD_BUG_ON(sizeof(ctx.nonce) != FS_KEY_DERIVATION_NONCE_SIZE);
 	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
+	if (ctx.version != FSCRYPT_CONTEXT_V1)
+		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
 
-	return inode->i_sb->s_cop->set_context(inode, &ctx, sizeof(ctx), NULL);
+	return inode->i_sb->s_cop->set_context(inode, &ctx,
+					       fscrypt_context_size(&ctx),
+					       NULL);
 }
 
 int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
@@ -67,7 +94,8 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 	if (!inode_owner_or_capable(inode))
 		return -EACCES;
 
-	if (policy.version != 0)
+	if (policy.version != FS_POLICY_VERSION_ORIGINAL &&
+	    policy.version != FS_POLICY_VERSION_HKDF)
 		return -EINVAL;
 
 	ret = mnt_want_write_file(filp);
@@ -85,7 +113,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 		else
 			ret = create_encryption_context_from_policy(inode,
 								    &policy);
-	} else if (ret == sizeof(ctx) &&
+	} else if (ret >= 0 && fscrypt_valid_context_format(&ctx, ret) &&
 		   is_encryption_context_consistent_with_policy(&ctx,
 								&policy)) {
 		/* The file already uses the same encryption policy. */
@@ -115,12 +143,10 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
 	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
 	if (res < 0 && res != -ERANGE)
 		return res;
-	if (res != sizeof(ctx))
-		return -EINVAL;
-	if (ctx.format != FS_ENCRYPTION_CONTEXT_FORMAT_V1)
+	if (res < 0 || !fscrypt_valid_context_format(&ctx, res))
 		return -EINVAL;
 
-	policy.version = 0;
+	policy.version = policy_version_for_context(&ctx);
 	policy.contents_encryption_mode = ctx.contents_encryption_mode;
 	policy.filenames_encryption_mode = ctx.filenames_encryption_mode;
 	policy.flags = ctx.flags;
@@ -200,6 +226,8 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 	if (parent_ci && child_ci) {
 		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
 			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+			(parent_ci->ci_context_version ==
+			 child_ci->ci_context_version) &&
 			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
 			(parent_ci->ci_filename_mode ==
 			 child_ci->ci_filename_mode) &&
@@ -207,16 +235,17 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 	}
 
 	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
-	if (res != sizeof(parent_ctx))
+	if (res < 0 || !fscrypt_valid_context_format(&parent_ctx, res))
 		return 0;
 
 	res = cops->get_context(child, &child_ctx, sizeof(child_ctx));
-	if (res != sizeof(child_ctx))
+	if (res < 0 || !fscrypt_valid_context_format(&child_ctx, res))
 		return 0;
 
 	return memcmp(parent_ctx.master_key_descriptor,
 		      child_ctx.master_key_descriptor,
 		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+		(parent_ctx.version == child_ctx.version) &&
 		(parent_ctx.contents_encryption_mode ==
 		 child_ctx.contents_encryption_mode) &&
 		(parent_ctx.filenames_encryption_mode ==
@@ -249,16 +278,20 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 	if (ci == NULL)
 		return -ENOKEY;
 
-	ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1;
+	ctx.version = ci->ci_context_version;
 	ctx.contents_encryption_mode = ci->ci_data_mode;
 	ctx.filenames_encryption_mode = ci->ci_filename_mode;
 	ctx.flags = ci->ci_flags;
 	memcpy(ctx.master_key_descriptor, ci->ci_master_key,
 	       FS_KEY_DESCRIPTOR_SIZE);
 	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
+	if (ctx.version != FSCRYPT_CONTEXT_V1)
+		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
+
 	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
 	res = parent->i_sb->s_cop->set_context(child, &ctx,
-						sizeof(ctx), fs_data);
+					       fscrypt_context_size(&ctx),
+					       fs_data);
 	if (res)
 		return res;
 	return preload ? fscrypt_get_encryption_info(child): 0;
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
index 97f738628b36..c08e8ae63a02 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt_common.h
@@ -84,7 +84,7 @@ struct fscrypt_operations {
 };
 
 /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
-#define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
+#define FSCRYPT_SET_CONTEXT_MAX_SIZE	44
 
 static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 {
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b7495d05e8de..a5423ddd3b67 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -257,6 +257,12 @@ struct fsxattr {
  * File system encryption support
  */
 /* Policy provided via an ioctl on the topmost directory */
+
+/* original policy version, no key verification (potentially insecure) */
+#define FS_POLICY_VERSION_ORIGINAL	0
+/* new version w/ HKDF and key verification (recommended) */
+#define FS_POLICY_VERSION_HKDF		2
+
 #define FS_KEY_DESCRIPTOR_SIZE	8
 
 #define FS_POLICY_FLAGS_PAD_4		0x00
-- 
2.13.2.932.g7449e964c-goog

Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, the fscrypt_context (i.e. the encryption xattr) does not
> contain a cryptographically secure identifier for the master key's
> payload.  Therefore it's not possible to verify that the correct key was
> supplied, which is problematic in multi-user scenarios.  To make this
> possible, define a new fscrypt_context version (v2) which includes a
> key_hash field, and allow userspace to opt-in to it when setting an
> encryption policy by setting fscrypt_policy.version to 2.  For now just
> zero the new field; a later patch will start setting it for real.

The main concern that comes to mind is potentially blowing past the
inline xattr size limit and allocating a new inode block.  The
security benefit probably outweighs that concern in this case.

> Even though we aren't changing the layout of struct fscrypt_policy (i.e.
> the struct used by the ioctls), the new context version still has to be
> "opt-in" because old kernels will not recognize it, and the keyring key
> will now need to be available when setting an encryption policy, which
> is an API change.  We'll also be taking the opportunity to make another
> API change (dropping support for the filesystem-specific key prefixes).
> 
> Previously, the version numbers were 0 in the fscrypt_policy and 1 in
> the fscrypt_context.  Rather than incrementing them to 1 and 2, make
> them both 2 to be consistent with each other.  It's not required that
> these numbers match, but it should make things less confusing.
> 
> An alternative to adding a key_hash field would have been to reuse
> master_key_descriptor.  However, master_key_descriptor is only 8 bytes,
> which is too short to be a cryptographically secure hash.  Thus,
> master_key_descriptor would have needed to be lengthened to 16+ bytes,
> which would have required defining a fscrypt_policy_v2 structure and
> adding a FS_IOC_GET_ENCRYPTION_POLICY_V2 ioctl.  It also would have
> required userspace to start using a specific hash algorithm to create
> the key descriptors, which would have made the API harder to use.
> Perhaps it should have been done that way originally, but at this point
> it seems better to keep the API simpler.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Michael Halcrow <mhalcrow@google.com>

> ---
>  fs/crypto/fscrypt_private.h    | 79 ++++++++++++++++++++++++++++++++++--------
>  fs/crypto/keyinfo.c            | 14 ++++----
>  fs/crypto/policy.c             | 67 ++++++++++++++++++++++++++---------
>  include/linux/fscrypt_common.h |  2 +-
>  include/uapi/linux/fs.h        |  6 ++++
>  5 files changed, 127 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index a1d5021c31ef..ef6909035823 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -25,39 +25,88 @@
>  #define FS_AES_256_XTS_KEY_SIZE		64
>  
>  #define FS_KEY_DERIVATION_NONCE_SIZE		16

I'm seeing tab misalignment from all the other values here.  Maybe
remove the extra tab while you're at it?

> +#define FSCRYPT_KEY_HASH_SIZE		16
>  
>  /**
> - * Encryption context for inode
> + * fscrypt_context - the encryption context for an inode
>   *
> - * Protector format:
> - *  1 byte: Protector format (1 = this version)
> - *  1 byte: File contents encryption mode
> - *  1 byte: File names encryption mode
> - *  1 byte: Flags
> - *  8 bytes: Master Key descriptor
> - *  16 bytes: Encryption Key derivation nonce
> + * Filesystems usually store this in an extended attribute.  It identifies the
> + * encryption algorithm and key with which the file is encrypted.
>   */
>  struct fscrypt_context {
> -	u8 format;
> +	/* v1+ */
> +
> +	/* Version of this structure */
> +	u8 version;
> +
> +	/* Encryption mode for the contents of regular files */
>  	u8 contents_encryption_mode;
> +
> +	/* Encryption mode for filenames in directories and symlink targets */
>  	u8 filenames_encryption_mode;
> +
> +	/* Options that affect how encryption is done (e.g. padding amount) */
>  	u8 flags;
> +
> +	/* Descriptor for this file's master key in the keyring */
>  	u8 master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
> +
> +	/*
> +	 * A unique value used in combination with the master key to derive the
> +	 * file's actual encryption key
> +	 */
>  	u8 nonce[FS_KEY_DERIVATION_NONCE_SIZE];
> -} __packed;
>  
> -#define FS_ENCRYPTION_CONTEXT_FORMAT_V1		1
> +	/* v2+ */
> +
> +	/* Cryptographically secure hash of the master key */
> +	u8 key_hash[FSCRYPT_KEY_HASH_SIZE];

Please add a comment not to re-order without macro changes below.

> +};
> +
> +#define FSCRYPT_CONTEXT_V1	1
> +#define FSCRYPT_CONTEXT_V1_SIZE	offsetof(struct fscrypt_context, key_hash)
> +
> +#define FSCRYPT_CONTEXT_V2	2
> +#define FSCRYPT_CONTEXT_V2_SIZE	sizeof(struct fscrypt_context)
> +
> +static inline int fscrypt_context_size(const struct fscrypt_context *ctx)
> +{
> +	switch (ctx->version) {
> +	case FSCRYPT_CONTEXT_V1:
> +		return FSCRYPT_CONTEXT_V1_SIZE;
> +	case FSCRYPT_CONTEXT_V2:
> +		return FSCRYPT_CONTEXT_V2_SIZE;
> +	}
> +	return 0;
> +}
> +
> +static inline bool
> +fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
> +{
> +	return size >= 1 && size == fscrypt_context_size(ctx);
> +}
>  
>  /*
> - * A pointer to this structure is stored in the file system's in-core
> - * representation of an inode.
> + * fscrypt_info - the "encryption key" for an inode
> + *
> + * When an encrypted file's key is made available, an instance of this struct is
> + * allocated and stored in ->i_crypt_info.  Once created, it remains until the
> + * inode is evicted.
>   */
>  struct fscrypt_info {
> +
> +	/* The actual crypto transforms needed for encryption and decryption */
> +	struct crypto_skcipher *ci_ctfm;
> +	struct crypto_cipher *ci_essiv_tfm;
> +
> +	/*
> +	 * Cached fields from the fscrypt_context needed for encryption policy
> +	 * inheritance and enforcement
> +	 */
> +	u8 ci_context_version;
>  	u8 ci_data_mode;
>  	u8 ci_filename_mode;
>  	u8 ci_flags;
> -	struct crypto_skcipher *ci_ctfm;
> -	struct crypto_cipher *ci_essiv_tfm;
>  	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
>  };
>  
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 018c588c7ac3..7e664a11340a 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -272,29 +272,27 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  			return res;
>  		/* Fake up a context for an unencrypted directory */
>  		memset(&ctx, 0, sizeof(ctx));
> -		ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1;
> +		ctx.version = FSCRYPT_CONTEXT_V1;
>  		ctx.contents_encryption_mode = FS_ENCRYPTION_MODE_AES_256_XTS;
>  		ctx.filenames_encryption_mode = FS_ENCRYPTION_MODE_AES_256_CTS;
>  		memset(ctx.master_key_descriptor, 0x42, FS_KEY_DESCRIPTOR_SIZE);
> -	} else if (res != sizeof(ctx)) {
> -		return -EINVAL;
> +		res = FSCRYPT_CONTEXT_V1_SIZE;
>  	}
>  
> -	if (ctx.format != FS_ENCRYPTION_CONTEXT_FORMAT_V1)
> +	if (!fscrypt_valid_context_format(&ctx, res))
>  		return -EINVAL;
>  
>  	if (ctx.flags & ~FS_POLICY_FLAGS_VALID)
>  		return -EINVAL;
>  
> -	crypt_info = kmem_cache_alloc(fscrypt_info_cachep, GFP_NOFS);
> +	crypt_info = kmem_cache_zalloc(fscrypt_info_cachep, GFP_NOFS);
>  	if (!crypt_info)
>  		return -ENOMEM;
>  
> -	crypt_info->ci_flags = ctx.flags;
> +	crypt_info->ci_context_version = ctx.version;
>  	crypt_info->ci_data_mode = ctx.contents_encryption_mode;
>  	crypt_info->ci_filename_mode = ctx.filenames_encryption_mode;
> -	crypt_info->ci_ctfm = NULL;
> -	crypt_info->ci_essiv_tfm = NULL;
> +	crypt_info->ci_flags = ctx.flags;
>  	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
>  				sizeof(crypt_info->ci_master_key));
>  
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index ce07a86200f3..044f23fadb5a 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -13,6 +13,28 @@
>  #include <linux/mount.h>
>  #include "fscrypt_private.h"
>  
> +static u8 policy_version_for_context(const struct fscrypt_context *ctx)
> +{
> +	switch (ctx->version) {
> +	case FSCRYPT_CONTEXT_V1:
> +		return FS_POLICY_VERSION_ORIGINAL;
> +	case FSCRYPT_CONTEXT_V2:
> +		return FS_POLICY_VERSION_HKDF;
> +	}
> +	BUG();
> +}
> +
> +static u8 context_version_for_policy(const struct fscrypt_policy *policy)
> +{
> +	switch (policy->version) {
> +	case FS_POLICY_VERSION_ORIGINAL:
> +		return FSCRYPT_CONTEXT_V1;
> +	case FS_POLICY_VERSION_HKDF:
> +		return FSCRYPT_CONTEXT_V2;
> +	}
> +	BUG();

Suggest commenting this function to require that policy be validated
prior to passing it here.  It's only called from
fscrypt_ioctl_set_policy() from what I can see, and that function
validates.

> +}
> +
>  /*
>   * check whether an encryption policy is consistent with an encryption context
>   */
> @@ -20,8 +42,10 @@ static bool is_encryption_context_consistent_with_policy(
>  				const struct fscrypt_context *ctx,
>  				const struct fscrypt_policy *policy)
>  {
> -	return memcmp(ctx->master_key_descriptor, policy->master_key_descriptor,
> -		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
> +	return (ctx->version == context_version_for_policy(policy)) &&
> +		(memcmp(ctx->master_key_descriptor,
> +			policy->master_key_descriptor,
> +			FS_KEY_DESCRIPTOR_SIZE) == 0) &&
>  		(ctx->flags == policy->flags) &&
>  		(ctx->contents_encryption_mode ==
>  		 policy->contents_encryption_mode) &&
> @@ -34,10 +58,6 @@ static int create_encryption_context_from_policy(struct inode *inode,
>  {
>  	struct fscrypt_context ctx;
>  
> -	ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1;
> -	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
> -					FS_KEY_DESCRIPTOR_SIZE);
> -
>  	if (!fscrypt_valid_enc_modes(policy->contents_encryption_mode,
>  				     policy->filenames_encryption_mode))
>  		return -EINVAL;
> @@ -45,13 +65,20 @@ static int create_encryption_context_from_policy(struct inode *inode,
>  	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
>  		return -EINVAL;
>  
> +	ctx.version = context_version_for_policy(policy);
>  	ctx.contents_encryption_mode = policy->contents_encryption_mode;
>  	ctx.filenames_encryption_mode = policy->filenames_encryption_mode;
>  	ctx.flags = policy->flags;
> +	memcpy(ctx.master_key_descriptor, policy->master_key_descriptor,
> +	       FS_KEY_DESCRIPTOR_SIZE);
>  	BUILD_BUG_ON(sizeof(ctx.nonce) != FS_KEY_DERIVATION_NONCE_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
> +	if (ctx.version != FSCRYPT_CONTEXT_V1)
> +		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
>  
> -	return inode->i_sb->s_cop->set_context(inode, &ctx, sizeof(ctx), NULL);
> +	return inode->i_sb->s_cop->set_context(inode, &ctx,
> +					       fscrypt_context_size(&ctx),
> +					       NULL);
>  }
>  
>  int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
> @@ -67,7 +94,8 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  	if (!inode_owner_or_capable(inode))
>  		return -EACCES;
>  
> -	if (policy.version != 0)
> +	if (policy.version != FS_POLICY_VERSION_ORIGINAL &&
> +	    policy.version != FS_POLICY_VERSION_HKDF)
>  		return -EINVAL;
>  
>  	ret = mnt_want_write_file(filp);
> @@ -85,7 +113,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  		else
>  			ret = create_encryption_context_from_policy(inode,
>  								    &policy);
> -	} else if (ret == sizeof(ctx) &&
> +	} else if (ret >= 0 && fscrypt_valid_context_format(&ctx, ret) &&
>  		   is_encryption_context_consistent_with_policy(&ctx,
>  								&policy)) {
>  		/* The file already uses the same encryption policy. */
> @@ -115,12 +143,10 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
>  	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
>  	if (res < 0 && res != -ERANGE)
>  		return res;
> -	if (res != sizeof(ctx))
> -		return -EINVAL;
> -	if (ctx.format != FS_ENCRYPTION_CONTEXT_FORMAT_V1)
> +	if (res < 0 || !fscrypt_valid_context_format(&ctx, res))
>  		return -EINVAL;

This ends up looking like a somewhat convoluted way of writing: "if
(res < 0) return res == -ERANGE ? -EINVAL : res;" Followed by the
check for context format validity.

Although there may be an even less convoluted way to write it.

>  
> -	policy.version = 0;
> +	policy.version = policy_version_for_context(&ctx);
>  	policy.contents_encryption_mode = ctx.contents_encryption_mode;
>  	policy.filenames_encryption_mode = ctx.filenames_encryption_mode;
>  	policy.flags = ctx.flags;
> @@ -200,6 +226,8 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  	if (parent_ci && child_ci) {
>  		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
>  			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
> +			(parent_ci->ci_context_version ==
> +			 child_ci->ci_context_version) &&
>  			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
>  			(parent_ci->ci_filename_mode ==
>  			 child_ci->ci_filename_mode) &&
> @@ -207,16 +235,17 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  	}
>  
>  	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
> -	if (res != sizeof(parent_ctx))
> +	if (res < 0 || !fscrypt_valid_context_format(&parent_ctx, res))
>  		return 0;
>  
>  	res = cops->get_context(child, &child_ctx, sizeof(child_ctx));
> -	if (res != sizeof(child_ctx))
> +	if (res < 0 || !fscrypt_valid_context_format(&child_ctx, res))
>  		return 0;
>  
>  	return memcmp(parent_ctx.master_key_descriptor,
>  		      child_ctx.master_key_descriptor,
>  		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
> +		(parent_ctx.version == child_ctx.version) &&
>  		(parent_ctx.contents_encryption_mode ==
>  		 child_ctx.contents_encryption_mode) &&
>  		(parent_ctx.filenames_encryption_mode ==
> @@ -249,16 +278,20 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  	if (ci == NULL)
>  		return -ENOKEY;
>  
> -	ctx.format = FS_ENCRYPTION_CONTEXT_FORMAT_V1;
> +	ctx.version = ci->ci_context_version;
>  	ctx.contents_encryption_mode = ci->ci_data_mode;
>  	ctx.filenames_encryption_mode = ci->ci_filename_mode;
>  	ctx.flags = ci->ci_flags;
>  	memcpy(ctx.master_key_descriptor, ci->ci_master_key,
>  	       FS_KEY_DESCRIPTOR_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
> +	if (ctx.version != FSCRYPT_CONTEXT_V1)
> +		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
> +
>  	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
>  	res = parent->i_sb->s_cop->set_context(child, &ctx,
> -						sizeof(ctx), fs_data);
> +					       fscrypt_context_size(&ctx),
> +					       fs_data);
>  	if (res)
>  		return res;
>  	return preload ? fscrypt_get_encryption_info(child): 0;
> diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt_common.h
> index 97f738628b36..c08e8ae63a02 100644
> --- a/include/linux/fscrypt_common.h
> +++ b/include/linux/fscrypt_common.h
> @@ -84,7 +84,7 @@ struct fscrypt_operations {
>  };
>  
>  /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
> -#define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
> +#define FSCRYPT_SET_CONTEXT_MAX_SIZE	44
>  
>  static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
>  {
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index b7495d05e8de..a5423ddd3b67 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -257,6 +257,12 @@ struct fsxattr {
>   * File system encryption support
>   */
>  /* Policy provided via an ioctl on the topmost directory */
> +
> +/* original policy version, no key verification (potentially insecure) */
> +#define FS_POLICY_VERSION_ORIGINAL	0
> +/* new version w/ HKDF and key verification (recommended) */
> +#define FS_POLICY_VERSION_HKDF		2
> +
>  #define FS_KEY_DESCRIPTOR_SIZE	8
>  
>  #define FS_POLICY_FLAGS_PAD_4		0x00
> -- 
> 2.13.2.932.g7449e964c-goog
> 

Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Eric Biggers]

Hi Michael,

On Thu, Jul 13, 2017 at 03:29:44PM -0700, Michael Halcrow wrote:
> On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Currently, the fscrypt_context (i.e. the encryption xattr) does not
> > contain a cryptographically secure identifier for the master key's
> > payload.  Therefore it's not possible to verify that the correct key was
> > supplied, which is problematic in multi-user scenarios.  To make this
> > possible, define a new fscrypt_context version (v2) which includes a
> > key_hash field, and allow userspace to opt-in to it when setting an
> > encryption policy by setting fscrypt_policy.version to 2.  For now just
> > zero the new field; a later patch will start setting it for real.
> 
> The main concern that comes to mind is potentially blowing past the
> inline xattr size limit and allocating a new inode block.  The
> security benefit probably outweighs that concern in this case.
> 

The way it adds up now for ext4 is:

128 bytes for base inode
+ 32 bytes for i_extra fields
+ 4 bytes for in-inode xattrs header
+ 20 bytes for encryption xattr header + name
+ 28 bytes for encryption xattr value
----------------------------------
= 212 bytes total.

By adding the 16-byte 'key_hash' field it grows to 228 bytes total.  So it still
fits in a 256-byte inode, though it's getting closer to the limit.  We could
save 8 bytes by instead using the design where master_key_descriptor is extended
to 16 bytes and redefined as a cryptographically secure hash.  But as noted,
that has some significant disadvantages.

Also note that we don't really have to worry about leaving space for a SELinux
xattr anymore because with 256-byte inodes + encryption the SELinux xattr is
already being written to an external block, given that it requires about 52-62
bytes (at least when using Android's SELinux policy; different SELinux policies
may use different values), and 212 + 52 > 256.  So if someone wants both xattrs
in-inode they need to use 512-byte inodes already.

Eric

Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

On Jul 13, 2017, at 3:58 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> Hi Michael,
> 
> On Thu, Jul 13, 2017 at 03:29:44PM -0700, Michael Halcrow wrote:
>> On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
>>> From: Eric Biggers <ebiggers@google.com>
>>> 
>>> Currently, the fscrypt_context (i.e. the encryption xattr) does not
>>> contain a cryptographically secure identifier for the master key's
>>> payload.  Therefore it's not possible to verify that the correct key was
>>> supplied, which is problematic in multi-user scenarios.  To make this
>>> possible, define a new fscrypt_context version (v2) which includes a
>>> key_hash field, and allow userspace to opt-in to it when setting an
>>> encryption policy by setting fscrypt_policy.version to 2.  For now just
>>> zero the new field; a later patch will start setting it for real.
>> 
>> The main concern that comes to mind is potentially blowing past the
>> inline xattr size limit and allocating a new inode block.  The
>> security benefit probably outweighs that concern in this case.
>> 
> 
> The way it adds up now for ext4 is:
> 
> 128 bytes for base inode
> + 32 bytes for i_extra fields
> + 4 bytes for in-inode xattrs header
> + 20 bytes for encryption xattr header + name
> + 28 bytes for encryption xattr value
> ----------------------------------
> = 212 bytes total.
> 
> By adding the 16-byte 'key_hash' field it grows to 228 bytes total.  So it still
> fits in a 256-byte inode, though it's getting closer to the limit.  We could
> save 8 bytes by instead using the design where master_key_descriptor is extended
> to 16 bytes and redefined as a cryptographically secure hash.  But as noted,
> that has some significant disadvantages.
> 
> Also note that we don't really have to worry about leaving space for a SELinux
> xattr anymore because with 256-byte inodes + encryption the SELinux xattr is
> already being written to an external block, given that it requires about 52-62
> bytes (at least when using Android's SELinux policy; different SELinux policies
> may use different values), and 212 + 52 > 256.  So if someone wants both xattrs
> in-inode they need to use 512-byte inodes already.

It is probably time to consider changing to a default of 512-byte inodes for
larger filesystems anyway.  In our testing, this affected performance only by
a couple of percent under normal usage, and avoided a significant performance
drop if the xattrs ever fall out of the inode.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH 2/6] fscrypt: rename ->ci_master_key to ->ci_master_key_descriptor

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

In struct fscrypt_info, ->ci_master_key is the master key descriptor,
not the master key itself.  In preparation for introducing a struct
fscrypt_master_key and making ->ci_master_key point to it, rename the
existing ->ci_master_key to ->ci_master_key_descriptor.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h | 2 +-
 fs/crypto/keyinfo.c         | 4 ++--
 fs/crypto/policy.c          | 5 +++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index ef6909035823..5470aac82cab 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -107,7 +107,7 @@ struct fscrypt_info {
 	u8 ci_data_mode;
 	u8 ci_filename_mode;
 	u8 ci_flags;
-	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
+	u8 ci_master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
 };
 
 typedef enum {
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 7e664a11340a..5591fd24e4b2 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -293,8 +293,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	crypt_info->ci_data_mode = ctx.contents_encryption_mode;
 	crypt_info->ci_filename_mode = ctx.filenames_encryption_mode;
 	crypt_info->ci_flags = ctx.flags;
-	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
-				sizeof(crypt_info->ci_master_key));
+	memcpy(crypt_info->ci_master_key_descriptor, ctx.master_key_descriptor,
+	       FS_KEY_DESCRIPTOR_SIZE);
 
 	res = determine_cipher_type(crypt_info, inode, &cipher_str, &keysize);
 	if (res)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 044f23fadb5a..81c59f8e45c0 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -224,7 +224,8 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 	child_ci = child->i_crypt_info;
 
 	if (parent_ci && child_ci) {
-		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
+		return memcmp(parent_ci->ci_master_key_descriptor,
+			      child_ci->ci_master_key_descriptor,
 			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
 			(parent_ci->ci_context_version ==
 			 child_ci->ci_context_version) &&
@@ -282,7 +283,7 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 	ctx.contents_encryption_mode = ci->ci_data_mode;
 	ctx.filenames_encryption_mode = ci->ci_filename_mode;
 	ctx.flags = ci->ci_flags;
-	memcpy(ctx.master_key_descriptor, ci->ci_master_key,
+	memcpy(ctx.master_key_descriptor, ci->ci_master_key_descriptor,
 	       FS_KEY_DESCRIPTOR_SIZE);
 	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
 	if (ctx.version != FSCRYPT_CONTEXT_V1)
-- 
2.13.2.932.g7449e964c-goog

Re: [PATCH 2/6] fscrypt: rename ->ci_master_key to ->ci_master_key_descriptor

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:31PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> In struct fscrypt_info, ->ci_master_key is the master key descriptor,
> not the master key itself.  In preparation for introducing a struct
> fscrypt_master_key and making ->ci_master_key point to it, rename the
> existing ->ci_master_key to ->ci_master_key_descriptor.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Michael Halcrow <mhalcrow@google.com>

> ---
>  fs/crypto/fscrypt_private.h | 2 +-
>  fs/crypto/keyinfo.c         | 4 ++--
>  fs/crypto/policy.c          | 5 +++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index ef6909035823..5470aac82cab 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -107,7 +107,7 @@ struct fscrypt_info {
>  	u8 ci_data_mode;
>  	u8 ci_filename_mode;
>  	u8 ci_flags;
> -	u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
> +	u8 ci_master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
>  };
>  
>  typedef enum {
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 7e664a11340a..5591fd24e4b2 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -293,8 +293,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	crypt_info->ci_data_mode = ctx.contents_encryption_mode;
>  	crypt_info->ci_filename_mode = ctx.filenames_encryption_mode;
>  	crypt_info->ci_flags = ctx.flags;
> -	memcpy(crypt_info->ci_master_key, ctx.master_key_descriptor,
> -				sizeof(crypt_info->ci_master_key));
> +	memcpy(crypt_info->ci_master_key_descriptor, ctx.master_key_descriptor,
> +	       FS_KEY_DESCRIPTOR_SIZE);
>  
>  	res = determine_cipher_type(crypt_info, inode, &cipher_str, &keysize);
>  	if (res)
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 044f23fadb5a..81c59f8e45c0 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -224,7 +224,8 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  	child_ci = child->i_crypt_info;
>  
>  	if (parent_ci && child_ci) {
> -		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
> +		return memcmp(parent_ci->ci_master_key_descriptor,
> +			      child_ci->ci_master_key_descriptor,
>  			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
>  			(parent_ci->ci_context_version ==
>  			 child_ci->ci_context_version) &&
> @@ -282,7 +283,7 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  	ctx.contents_encryption_mode = ci->ci_data_mode;
>  	ctx.filenames_encryption_mode = ci->ci_filename_mode;
>  	ctx.flags = ci->ci_flags;
> -	memcpy(ctx.master_key_descriptor, ci->ci_master_key,
> +	memcpy(ctx.master_key_descriptor, ci->ci_master_key_descriptor,
>  	       FS_KEY_DESCRIPTOR_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
>  	if (ctx.version != FSCRYPT_CONTEXT_V1)
> -- 
> 2.13.2.932.g7449e964c-goog
> 

[PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

By design, the keys which userspace provides in the keyring are not used
to encrypt data directly.  Instead, a KDF (Key Derivation Function) is
used to derive a unique encryption key for each inode, given a "master"
key and a nonce.  The current KDF encrypts the master key with
AES-128-ECB using the nonce as the AES key.  This KDF is ad-hoc and is
not specified in any standard.  While it does generate unique derived
keys with sufficient entropy, it has several disadvantages:

- It's reversible: an attacker who compromises a derived key, e.g. using
  a side channel attack, can "decrypt" it to get back to the master key.

- It's not very extensible because it cannot easily be used to derive
  other key material that may be needed and it ties the length of the
  derived key closely to the length of the master key.

- It doesn't evenly distribute the entropy from the master key.  For
  example, the first 16 bytes of each derived key depend only on the
  first 16 bytes of the master key.

- It uses a public value as an AES key, which is unusual.  Ciphers are
  rarely evaluated under a threat model where the keys are public and
  the messages are secret.

Solve all these problems for v2 encryption policies by changing the KDF
to HKDF with SHA-512 as the underlying hash function.  To derive each
inode's encryption key, HKDF is executed with the master key as the
input key material, a fixed salt, and the per-inode nonce prefixed with
a context byte as the application-specific information string.  Unlike
the current KDF, HKDF has been formally published and standardized
[1][2], is nonreversible, can be used to derive any number and length of
secret and/or non-secret keys, and evenly distributes the entropy from
the master key (excepting limits inherent to not using a random salt).

Note that this introduces a dependency on the security and
implementation of SHA-512, whereas before we were using only AES for
both key derivation and encryption.  However, by using HMAC rather than
the hash function directly, HKDF is designed to remain secure even if
various classes of attacks, e.g. collision attacks, are found against
the underlying unkeyed hash function.  Even HMAC-MD5 is still considered
secure in practice, despite MD5 itself having been heavily compromised.

We *could* avoid introducing a hash function by instantiating
HKDF-Expand with CMAC-AES256 as the pseudorandom function rather than
HMAC-SHA512.  This would work; however, the HKDF specification doesn't
explicitly allow a non-HMAC pseudorandom function, so it would be less
standard.  It would also require skipping HKDF-Extract and changing the
API to accept only 32-byte master keys (since otherwise HKDF-Extract
using CMAC-AES would produce a pseudorandom key only 16 bytes long which
is only enough for AES-128, not AES-256).

HKDF-SHA512 can require more "crypto work" per key derivation when
compared to the current KDF.  However, later in this series, we'll start
caching the HMAC transform for each master key, which will actually make
the real-world performance about the same or even significantly better
than the AES-based KDF as currently implemented.  Also, each KDF can
actually be executed on the order of 1 million times per second, so KDF
performance probably isn't actually the bottleneck in practice anyway.

References:
  [1] Krawczyk (2010). "Cryptographic Extraction and Key Derivation: The
      HKDF Scheme".  https://eprint.iacr.org/2010/264.pdf

  [2] RFC 5869. "HMAC-based Extract-and-Expand Key Derivation Function
      (HKDF)".  https://tools.ietf.org/html/rfc5869

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/Kconfig           |   2 +
 fs/crypto/fscrypt_private.h |  14 ++
 fs/crypto/keyinfo.c         | 485 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 405 insertions(+), 96 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 02b7d91c9231..bbd4e38b293c 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -8,6 +8,8 @@ config FS_ENCRYPTION
 	select CRYPTO_CTS
 	select CRYPTO_CTR
 	select CRYPTO_SHA256
+	select CRYPTO_SHA512
+	select CRYPTO_HMAC
 	select KEYS
 	help
 	  Enable encryption of files and directories.  This
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 5470aac82cab..095e7c16483a 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -86,6 +86,14 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
 	return size >= 1 && size == fscrypt_context_size(ctx);
 }
 
+/*
+ * fscrypt_master_key - an in-use master key
+ */
+struct fscrypt_master_key {
+	struct crypto_shash	*mk_hmac;
+	unsigned int		mk_size;
+};
+
 /*
  * fscrypt_info - the "encryption key" for an inode
  *
@@ -99,6 +107,12 @@ struct fscrypt_info {
 	struct crypto_skcipher *ci_ctfm;
 	struct crypto_cipher *ci_essiv_tfm;
 
+	/*
+	 * The master key with which this inode was "unlocked"
+	 * (only set for inodes that use a v2+ encryption policy)
+	 */
+	struct fscrypt_master_key *ci_master_key;
+
 	/*
 	 * Cached fields from the fscrypt_context needed for encryption policy
 	 * inheritance and enforcement
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 5591fd24e4b2..7ed1a7fb1308 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -6,17 +6,312 @@
  * This contains encryption key functions.
  *
  * Written by Michael Halcrow, Ildar Muslukhov, and Uday Savagaonkar, 2015.
+ * HKDF support added by Eric Biggers, 2017.
+ *
+ * The implementation and usage of HKDF should conform to RFC-5869 ("HMAC-based
+ * Extract-and-Expand Key Derivation Function").
  */
 
 #include <keys/user-type.h>
 #include <linux/scatterlist.h>
 #include <linux/ratelimit.h>
 #include <crypto/aes.h>
+#include <crypto/hash.h>
 #include <crypto/sha.h>
 #include "fscrypt_private.h"
 
 static struct crypto_shash *essiv_hash_tfm;
 
+/*
+ * Any unkeyed cryptographic hash algorithm can be used with HKDF, but we use
+ * SHA-512 because it is reasonably secure and efficient; and since it produces
+ * a 64-byte digest, deriving an AES-256-XTS key preserves all 64 bytes of
+ * entropy from the master key and requires only one iteration of HKDF-Expand.
+ */
+#define HKDF_HMAC_ALG		"hmac(sha512)"
+#define HKDF_HASHLEN		SHA512_DIGEST_SIZE
+
+/*
+ * The list of contexts in which we use HKDF to derive additional keys from a
+ * master key.  The values in this list are used as the first byte of the
+ * application-specific info string to guarantee that info strings are never
+ * repeated between contexts.
+ *
+ * Keys derived with different info strings are cryptographically isolated from
+ * each other --- knowledge of one derived key doesn't reveal any others.
+ */
+#define HKDF_CONTEXT_PER_FILE_KEY	1
+
+/*
+ * HKDF consists of two steps:
+ *
+ * 1. HKDF-Extract: extract a fixed-length pseudorandom key from the
+ *    input keying material and optional salt.
+ * 2. HDKF-Expand: expand the pseudorandom key into output keying material of
+ *    any length, parameterized by an application-specific info string.
+ *
+ * HKDF-Extract can be skipped if the input is already a good pseudorandom key
+ * that is at least as long as the hash.  While the fscrypt master keys should
+ * already be good pseudorandom keys, when using encryption algorithms that use
+ * short keys (e.g. AES-128-CBC) we'd like to permit the master key to be
+ * shorter than HKDF_HASHLEN bytes.  Thus, we still must do HKDF-Extract.
+ *
+ * Ideally, HKDF-Extract would be passed a random salt for each distinct input
+ * key.  Details about the advantages of a random salt can be found in the HKDF
+ * paper (Krawczyk, 2010; "Cryptographic Extraction and Key Derivation: The HKDF
+ * Scheme").  However, we do not have the ability to store a salt on a
+ * per-master-key basis.  Thus, we have to use a fixed salt.  This is sufficient
+ * as long as the master keys are already pseudorandom and are long enough to
+ * make dictionary attacks infeasible.  This should be the case if userspace
+ * used a cryptographically secure random number generator, e.g. /dev/urandom,
+ * to generate the master keys.
+ *
+ * For the fixed salt we use "fscrypt_hkdf_salt" rather than default of all 0's
+ * defined by RFC-5869.  This is only to be slightly more robust against
+ * userspace (unwisely) reusing the master keys for different purposes.
+ * Logically, it's more likely that the keys would be passed to unsalted
+ * HKDF-SHA512 than specifically to "fscrypt_hkdf_salt"-salted HKDF-SHA512.
+ * (Of course, a random salt would be better for this purpose.)
+ */
+
+#define HKDF_SALT		"fscrypt_hkdf_salt"
+#define HKDF_SALT_LEN		(sizeof(HKDF_SALT) - 1)
+
+/*
+ * HKDF-Extract (RFC-5869 section 2.2).  This extracts a pseudorandom key 'prk'
+ * from the input key material 'ikm' and a salt.  (See explanation above for why
+ * we use a fixed salt.)
+ */
+static int hkdf_extract(struct crypto_shash *hmac,
+			const u8 *ikm, unsigned int ikmlen,
+			u8 prk[HKDF_HASHLEN])
+{
+	SHASH_DESC_ON_STACK(desc, hmac);
+	int err;
+
+	desc->tfm = hmac;
+	desc->flags = 0;
+
+	err = crypto_shash_setkey(hmac, HKDF_SALT, HKDF_SALT_LEN);
+	if (err)
+		goto out;
+
+	err = crypto_shash_digest(desc, ikm, ikmlen, prk);
+out:
+	shash_desc_zero(desc);
+	return err;
+}
+
+/*
+ * HKDF-Expand (RFC-5869 section 2.3).  This expands the pseudorandom key, which
+ * has already been keyed into 'hmac', into 'okmlen' bytes of output keying
+ * material, parameterized by the application-specific information string of
+ * 'info' prefixed with the 'context' byte.  ('context' isn't part of the HKDF
+ * specification; it's just a prefix we add to our application-specific info
+ * strings to guarantee that we don't accidentally repeat an info string when
+ * using HKDF for different purposes.)
+ */
+static int hkdf_expand(struct crypto_shash *hmac, u8 context,
+		       const u8 *info, unsigned int infolen,
+		       u8 *okm, unsigned int okmlen)
+{
+	SHASH_DESC_ON_STACK(desc, hmac);
+	int err;
+	const u8 *prev = NULL;
+	unsigned int i;
+	u8 counter = 1;
+	u8 tmp[HKDF_HASHLEN];
+
+	desc->tfm = hmac;
+	desc->flags = 0;
+
+	if (unlikely(okmlen > 255 * HKDF_HASHLEN))
+		return -EINVAL;
+
+	for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
+
+		err = crypto_shash_init(desc);
+		if (err)
+			goto out;
+
+		if (prev) {
+			err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
+			if (err)
+				goto out;
+		}
+
+		err = crypto_shash_update(desc, &context, 1);
+		if (err)
+			goto out;
+
+		err = crypto_shash_update(desc, info, infolen);
+		if (err)
+			goto out;
+
+		if (okmlen - i < HKDF_HASHLEN) {
+			err = crypto_shash_finup(desc, &counter, 1, tmp);
+			if (err)
+				goto out;
+			memcpy(&okm[i], tmp, okmlen - i);
+			memzero_explicit(tmp, sizeof(tmp));
+		} else {
+			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
+			if (err)
+				goto out;
+		}
+		counter++;
+		prev = &okm[i];
+	}
+	err = 0;
+out:
+	shash_desc_zero(desc);
+	return err;
+}
+
+static void put_master_key(struct fscrypt_master_key *k)
+{
+	if (!k)
+		return;
+
+	crypto_free_shash(k->mk_hmac);
+	kzfree(k);
+}
+
+/*
+ * Allocate a fscrypt_master_key, given the keyring key payload.  This includes
+ * allocating and keying an HMAC transform so that we can efficiently derive
+ * the per-inode encryption keys with HKDF-Expand later.
+ */
+static struct fscrypt_master_key *
+alloc_master_key(const struct fscrypt_key *payload)
+{
+	struct fscrypt_master_key *k;
+	int err;
+	u8 prk[HKDF_HASHLEN];
+
+	k = kzalloc(sizeof(*k), GFP_NOFS);
+	if (!k)
+		return ERR_PTR(-ENOMEM);
+	k->mk_size = payload->size;
+
+	k->mk_hmac = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0);
+	if (IS_ERR(k->mk_hmac)) {
+		err = PTR_ERR(k->mk_hmac);
+		k->mk_hmac = NULL;
+		pr_warn("fscrypt: error allocating " HKDF_HMAC_ALG ": %d\n",
+			err);
+		goto fail;
+	}
+
+	BUG_ON(crypto_shash_digestsize(k->mk_hmac) != sizeof(prk));
+
+	err = hkdf_extract(k->mk_hmac, payload->raw, payload->size, prk);
+	if (err)
+		goto fail;
+
+	err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
+	if (err)
+		goto fail;
+out:
+	memzero_explicit(prk, sizeof(prk));
+	return k;
+
+fail:
+	put_master_key(k);
+	k = ERR_PTR(err);
+	goto out;
+}
+
+static void release_keyring_key(struct key *keyring_key)
+{
+	up_read(&keyring_key->sem);
+	key_put(keyring_key);
+}
+
+/*
+ * Find, lock, and validate the master key with the keyring description
+ * prefix:descriptor.  It must be released with release_keyring_key() later.
+ */
+static struct key *
+find_and_lock_keyring_key(const char *prefix,
+			  const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
+			  unsigned int min_keysize,
+			  const struct fscrypt_key **payload_ret)
+{
+	char *description;
+	struct key *keyring_key;
+	const struct user_key_payload *ukp;
+	const struct fscrypt_key *payload;
+
+	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
+				FS_KEY_DESCRIPTOR_SIZE, descriptor);
+	if (!description)
+		return ERR_PTR(-ENOMEM);
+
+	keyring_key = request_key(&key_type_logon, description, NULL);
+	if (IS_ERR(keyring_key))
+		goto out;
+
+	down_read(&keyring_key->sem);
+	ukp = user_key_payload_locked(keyring_key);
+	payload = (const struct fscrypt_key *)ukp->data;
+
+	if (ukp->datalen != sizeof(struct fscrypt_key) ||
+	    payload->size == 0 || payload->size > FS_MAX_KEY_SIZE) {
+		pr_warn_ratelimited("fscrypt: key '%s' has invalid payload\n",
+				    description);
+		goto invalid;
+	}
+
+	/*
+	 * With the legacy AES-based KDF the master key must be at least as long
+	 * as the derived key.  With HKDF we could accept a shorter master key;
+	 * however, that would mean the derived key wouldn't contain as much
+	 * entropy as intended.  So don't allow it in either case.
+	 */
+	if (payload->size < min_keysize) {
+		pr_warn_ratelimited("fscrypt: key '%s' is too short (got %u bytes, wanted %u+ bytes)\n",
+				    description, payload->size, min_keysize);
+		goto invalid;
+	}
+
+	*payload_ret = payload;
+out:
+	kfree(description);
+	return keyring_key;
+
+invalid:
+	release_keyring_key(keyring_key);
+	keyring_key = ERR_PTR(-ENOKEY);
+	goto out;
+}
+
+static struct fscrypt_master_key *
+load_master_key_from_keyring(const struct inode *inode,
+			     const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
+			     unsigned int min_keysize)
+{
+	struct key *keyring_key;
+	const struct fscrypt_key *payload;
+	struct fscrypt_master_key *master_key;
+
+	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX, descriptor,
+						min_keysize, &payload);
+	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
+		keyring_key = find_and_lock_keyring_key(
+					inode->i_sb->s_cop->key_prefix,
+					descriptor, min_keysize, &payload);
+	}
+	if (IS_ERR(keyring_key))
+		return ERR_CAST(keyring_key);
+
+	master_key = alloc_master_key(payload);
+
+	release_keyring_key(keyring_key);
+
+	return master_key;
+}
+
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
 {
 	struct fscrypt_completion_result *ecr = req->data;
@@ -28,107 +323,100 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
 	complete(&ecr->completion);
 }
 
-/**
- * derive_key_aes() - Derive a key using AES-128-ECB
- * @deriving_key: Encryption key used for derivation.
- * @source_key:   Source key to which to apply derivation.
- * @derived_raw_key:  Derived raw key.
- *
- * Return: Zero on success; non-zero otherwise.
+/*
+ * Legacy key derivation function.  This generates the derived key by encrypting
+ * the master key with AES-128-ECB using the nonce as the AES key.  This
+ * provides a unique derived key for each inode, but it's nonstandard, isn't
+ * very extensible, and has the weakness that it's trivially reversible: an
+ * attacker who compromises a derived key, e.g. with a side channel attack, can
+ * "decrypt" it to get back to the master key, then derive any other key.
  */
-static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
-				const struct fscrypt_key *source_key,
-				u8 derived_raw_key[FS_MAX_KEY_SIZE])
+static int derive_key_aes(const struct fscrypt_key *master_key,
+			  const struct fscrypt_context *ctx,
+			  u8 *derived_key, unsigned int derived_keysize)
 {
-	int res = 0;
+	int err;
 	struct skcipher_request *req = NULL;
 	DECLARE_FS_COMPLETION_RESULT(ecr);
 	struct scatterlist src_sg, dst_sg;
-	struct crypto_skcipher *tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
+	struct crypto_skcipher *tfm;
+
+	tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
 
-	if (IS_ERR(tfm)) {
-		res = PTR_ERR(tfm);
-		tfm = NULL;
-		goto out;
-	}
 	crypto_skcipher_set_flags(tfm, CRYPTO_TFM_REQ_WEAK_KEY);
 	req = skcipher_request_alloc(tfm, GFP_NOFS);
 	if (!req) {
-		res = -ENOMEM;
+		err = -ENOMEM;
 		goto out;
 	}
 	skcipher_request_set_callback(req,
 			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
 			derive_crypt_complete, &ecr);
-	res = crypto_skcipher_setkey(tfm, deriving_key,
-					FS_AES_128_ECB_KEY_SIZE);
-	if (res < 0)
+
+	BUILD_BUG_ON(sizeof(ctx->nonce) != FS_AES_128_ECB_KEY_SIZE);
+	err = crypto_skcipher_setkey(tfm, ctx->nonce, sizeof(ctx->nonce));
+	if (err)
 		goto out;
 
-	sg_init_one(&src_sg, source_key->raw, source_key->size);
-	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
+	sg_init_one(&src_sg, master_key->raw, derived_keysize);
+	sg_init_one(&dst_sg, derived_key, derived_keysize);
+	skcipher_request_set_crypt(req, &src_sg, &dst_sg, derived_keysize,
 				   NULL);
-	res = crypto_skcipher_encrypt(req);
-	if (res == -EINPROGRESS || res == -EBUSY) {
+	err = crypto_skcipher_encrypt(req);
+	if (err == -EINPROGRESS || err == -EBUSY) {
 		wait_for_completion(&ecr.completion);
-		res = ecr.res;
+		err = ecr.res;
 	}
 out:
 	skcipher_request_free(req);
 	crypto_free_skcipher(tfm);
-	return res;
+	return err;
 }
 
-static int validate_user_key(struct fscrypt_info *crypt_info,
-			struct fscrypt_context *ctx, u8 *raw_key,
-			const char *prefix, int min_keysize)
+/*
+ * HKDF-based key derivation function.  This uses HKDF-SHA512 to derive a unique
+ * encryption key for each inode, using the inode's nonce prefixed with a
+ * context byte as the application-specific information string.  This is more
+ * flexible than the legacy AES-based KDF and has the advantage that it's
+ * non-reversible: an attacker who compromises a derived key cannot calculate
+ * the master key or any other derived keys.
+ */
+static int derive_key_hkdf(const struct fscrypt_master_key *master_key,
+			   const struct fscrypt_context *ctx,
+			   u8 *derived_key, unsigned int derived_keysize)
 {
-	char *description;
-	struct key *keyring_key;
-	struct fscrypt_key *master_key;
-	const struct user_key_payload *ukp;
-	int res;
+	return hkdf_expand(master_key->mk_hmac, HKDF_CONTEXT_PER_FILE_KEY,
+			   ctx->nonce, sizeof(ctx->nonce),
+			   derived_key, derived_keysize);
+}
 
-	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
-				FS_KEY_DESCRIPTOR_SIZE,
-				ctx->master_key_descriptor);
-	if (!description)
-		return -ENOMEM;
+static int find_and_derive_key_v1(const struct inode *inode,
+				  const struct fscrypt_context *ctx,
+				  u8 *derived_key, unsigned int derived_keysize)
+{
+	struct key *keyring_key;
+	const struct fscrypt_key *payload;
+	int err;
 
-	keyring_key = request_key(&key_type_logon, description, NULL);
-	kfree(description);
+	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX,
+						ctx->master_key_descriptor,
+						derived_keysize, &payload);
+	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
+		keyring_key = find_and_lock_keyring_key(
+					inode->i_sb->s_cop->key_prefix,
+					ctx->master_key_descriptor,
+					derived_keysize, &payload);
+	}
 	if (IS_ERR(keyring_key))
 		return PTR_ERR(keyring_key);
-	down_read(&keyring_key->sem);
 
-	if (keyring_key->type != &key_type_logon) {
-		printk_once(KERN_WARNING
-				"%s: key type must be logon\n", __func__);
-		res = -ENOKEY;
-		goto out;
-	}
-	ukp = user_key_payload_locked(keyring_key);
-	if (ukp->datalen != sizeof(struct fscrypt_key)) {
-		res = -EINVAL;
-		goto out;
-	}
-	master_key = (struct fscrypt_key *)ukp->data;
-	BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
-
-	if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
-	    || master_key->size % AES_BLOCK_SIZE != 0) {
-		printk_once(KERN_WARNING
-				"%s: key size incorrect: %d\n",
-				__func__, master_key->size);
-		res = -ENOKEY;
-		goto out;
-	}
-	res = derive_key_aes(ctx->nonce, master_key, raw_key);
-out:
-	up_read(&keyring_key->sem);
-	key_put(keyring_key);
-	return res;
+	err = derive_key_aes(payload, ctx, derived_key, derived_keysize);
+
+	release_keyring_key(keyring_key);
+
+	return err;
 }
 
 static const struct {
@@ -179,6 +467,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
 
 	crypto_free_skcipher(ci->ci_ctfm);
 	crypto_free_cipher(ci->ci_essiv_tfm);
+	put_master_key(ci->ci_master_key);
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
 
@@ -254,8 +543,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	struct fscrypt_context ctx;
 	struct crypto_skcipher *ctfm;
 	const char *cipher_str;
-	int keysize;
-	u8 *raw_key = NULL;
+	unsigned int derived_keysize;
+	u8 *derived_key = NULL;
 	int res;
 
 	if (inode->i_crypt_info)
@@ -296,33 +585,40 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	memcpy(crypt_info->ci_master_key_descriptor, ctx.master_key_descriptor,
 	       FS_KEY_DESCRIPTOR_SIZE);
 
-	res = determine_cipher_type(crypt_info, inode, &cipher_str, &keysize);
+	res = determine_cipher_type(crypt_info, inode, &cipher_str,
+				    &derived_keysize);
 	if (res)
 		goto out;
 
 	/*
-	 * This cannot be a stack buffer because it is passed to the scatterlist
-	 * crypto API as part of key derivation.
+	 * This cannot be a stack buffer because it may be passed to the
+	 * scatterlist crypto API during key derivation.
 	 */
 	res = -ENOMEM;
-	raw_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
-	if (!raw_key)
+	derived_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
+	if (!derived_key)
 		goto out;
 
-	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
-				keysize);
-	if (res && inode->i_sb->s_cop->key_prefix) {
-		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
-					     inode->i_sb->s_cop->key_prefix,
-					     keysize);
-		if (res2) {
-			if (res2 == -ENOKEY)
-				res = -ENOKEY;
+	if (ctx.version == FSCRYPT_CONTEXT_V1) {
+		res = find_and_derive_key_v1(inode, &ctx, derived_key,
+					     derived_keysize);
+	} else {
+		crypt_info->ci_master_key =
+			load_master_key_from_keyring(inode,
+						     ctx.master_key_descriptor,
+						     derived_keysize);
+		if (IS_ERR(crypt_info->ci_master_key)) {
+			res = PTR_ERR(crypt_info->ci_master_key);
+			crypt_info->ci_master_key = NULL;
 			goto out;
 		}
-	} else if (res) {
-		goto out;
+
+		res = derive_key_hkdf(crypt_info->ci_master_key, &ctx,
+				      derived_key, derived_keysize);
 	}
+	if (res)
+		goto out;
+
 	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
 	if (!ctfm || IS_ERR(ctfm)) {
 		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
@@ -333,17 +629,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	crypt_info->ci_ctfm = ctfm;
 	crypto_skcipher_clear_flags(ctfm, ~0);
 	crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_REQ_WEAK_KEY);
-	/*
-	 * if the provided key is longer than keysize, we use the first
-	 * keysize bytes of the derived key only
-	 */
-	res = crypto_skcipher_setkey(ctfm, raw_key, keysize);
+	res = crypto_skcipher_setkey(ctfm, derived_key, derived_keysize);
 	if (res)
 		goto out;
 
 	if (S_ISREG(inode->i_mode) &&
 	    crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
-		res = init_essiv_generator(crypt_info, raw_key, keysize);
+		res = init_essiv_generator(crypt_info, derived_key,
+					   derived_keysize);
 		if (res) {
 			pr_debug("%s: error %d (inode %lu) allocating essiv tfm\n",
 				 __func__, res, inode->i_ino);
@@ -356,7 +649,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	if (res == -ENOKEY)
 		res = 0;
 	put_crypt_info(crypt_info);
-	kzfree(raw_key);
+	kzfree(derived_key);
 	return res;
 }
 EXPORT_SYMBOL(fscrypt_get_encryption_info);
-- 
2.13.2.932.g7449e964c-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Stephan Müller]

Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:

Hi Herbert,

This patch adds a second KDF to the kernel -- the first is found in the keys 
subsystem.

The next KDF that may come in is in the TLS scope.

Would it make sense to warm up the KDF patches adding generic KDF support to 
the kernel crypto API that I supplied some time ago? The advantages would be 
to have one location of KDF implementations and the benefit of the testmgr.

Ciao
Stephan

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Herbert Xu]

On Thu, Jul 13, 2017 at 04:54:55PM +0200, Stephan Müller wrote:
> Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:
> 
> Hi Herbert,
> 
> This patch adds a second KDF to the kernel -- the first is found in the keys 
> subsystem.
> 
> The next KDF that may come in is in the TLS scope.
> 
> Would it make sense to warm up the KDF patches adding generic KDF support to 
> the kernel crypto API that I supplied some time ago? The advantages would be 
> to have one location of KDF implementations and the benefit of the testmgr.

Sure.  Though I'd like to see what it looks like before I commit :)

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Stephan Müller]

Am Donnerstag, 13. Juli 2017, 18:07:54 CEST schrieb Herbert Xu:

Hi Herbert,

> Sure.  Though I'd like to see what it looks like before I commit :)

Naturally. :-)

The patches would create an RNG template support. KDFs are not more than 
special-purpose RNGs.

Ciao
Stephan

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Eric Biggers]

Hi Stephan,

On Thu, Jul 13, 2017 at 04:54:55PM +0200, Stephan Müller wrote:
> Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:
> 
> Hi Herbert,
> 
> This patch adds a second KDF to the kernel -- the first is found in the keys 
> subsystem.
> 
> The next KDF that may come in is in the TLS scope.
> 
> Would it make sense to warm up the KDF patches adding generic KDF support to 
> the kernel crypto API that I supplied some time ago? The advantages would be 
> to have one location of KDF implementations and the benefit of the testmgr.
> 

That may be a good idea.  Looking at the old thread, I share Herbert's concern
(http://www.spinics.net/lists/linux-crypto/msg21231.html) about there likely not
being more than one implementation of each KDF algorithm.  So, perhaps some
simple helper functions would be more appropriate.  However, making the KDFs be
covered by self-tests would be very nice.

Also, it seems your patch
(http://www.spinics.net/lists/linux-crypto/msg21137.html) doesn't allow a salt
to be passed in.  In order to fully support HKDF, crypto_rng_reset() (which as I
understand would be the way to invoke the "extract" step) would somehow need to
accept both the input keying material and salt, both of which are arbitrary
length binary.

Eric

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Stephan Müller]

Am Donnerstag, 13. Juli 2017, 20:10:57 CEST schrieb Eric Biggers:

Hi Eric,

> Hi Stephan,
> 
> On Thu, Jul 13, 2017 at 04:54:55PM +0200, Stephan Müller wrote:
> > Am Mittwoch, 12. Juli 2017, 23:00:32 CEST schrieb Eric Biggers:
> > 
> > Hi Herbert,
> > 
> > This patch adds a second KDF to the kernel -- the first is found in the
> > keys subsystem.
> > 
> > The next KDF that may come in is in the TLS scope.
> > 
> > Would it make sense to warm up the KDF patches adding generic KDF support
> > to the kernel crypto API that I supplied some time ago? The advantages
> > would be to have one location of KDF implementations and the benefit of
> > the testmgr.
> That may be a good idea.  Looking at the old thread, I share Herbert's
> concern (http://www.spinics.net/lists/linux-crypto/msg21231.html) about
> there likely not being more than one implementation of each KDF algorithm. 
> So, perhaps some simple helper functions would be more appropriate. 
> However, making the KDFs be covered by self-tests would be very nice.

I agree that it is likely that specific KDF implementations may only be used 
once. But still, I would recommend to maintain those implementation under the 
crypto API umbrella, as KDFs are cryptographic operations.

> 
> Also, it seems your patch
> (http://www.spinics.net/lists/linux-crypto/msg21137.html) doesn't allow a
> salt to be passed in.  In order to fully support HKDF, crypto_rng_reset()
> (which as I understand would be the way to invoke the "extract" step) would
> somehow need to accept both the input keying material and salt, both of
> which are arbitrary length binary.

I concur with you. I have implemented the HKDF in my libkcapi as well and saw 
the need for a salt.

Let me work on an update to the KDF patch for the kernel crypto API.

Ciao
Stephan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:32PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> By design, the keys which userspace provides in the keyring are not used
> to encrypt data directly.  Instead, a KDF (Key Derivation Function) is
> used to derive a unique encryption key for each inode, given a "master"
> key and a nonce.  The current KDF encrypts the master key with
> AES-128-ECB using the nonce as the AES key.  This KDF is ad-hoc and is
> not specified in any standard.  While it does generate unique derived
> keys with sufficient entropy, it has several disadvantages:
> 
> - It's reversible: an attacker who compromises a derived key, e.g. using
>   a side channel attack, can "decrypt" it to get back to the master key.
> 
> - It's not very extensible because it cannot easily be used to derive
>   other key material that may be needed and it ties the length of the
>   derived key closely to the length of the master key.
> 
> - It doesn't evenly distribute the entropy from the master key.  For
>   example, the first 16 bytes of each derived key depend only on the
>   first 16 bytes of the master key.
> 
> - It uses a public value as an AES key, which is unusual.  Ciphers are
>   rarely evaluated under a threat model where the keys are public and
>   the messages are secret.
> 
> Solve all these problems for v2 encryption policies by changing the KDF
> to HKDF with SHA-512 as the underlying hash function.  To derive each
> inode's encryption key, HKDF is executed with the master key as the
> input key material, a fixed salt, and the per-inode nonce prefixed with
> a context byte as the application-specific information string.  Unlike
> the current KDF, HKDF has been formally published and standardized
> [1][2], is nonreversible, can be used to derive any number and length of
> secret and/or non-secret keys, and evenly distributes the entropy from
> the master key (excepting limits inherent to not using a random salt).
> 
> Note that this introduces a dependency on the security and
> implementation of SHA-512, whereas before we were using only AES for
> both key derivation and encryption.  However, by using HMAC rather than
> the hash function directly, HKDF is designed to remain secure even if
> various classes of attacks, e.g. collision attacks, are found against
> the underlying unkeyed hash function.  Even HMAC-MD5 is still considered
> secure in practice, despite MD5 itself having been heavily compromised.
> 
> We *could* avoid introducing a hash function by instantiating
> HKDF-Expand with CMAC-AES256 as the pseudorandom function rather than
> HMAC-SHA512.  This would work; however, the HKDF specification doesn't
> explicitly allow a non-HMAC pseudorandom function, so it would be less
> standard.  It would also require skipping HKDF-Extract and changing the
> API to accept only 32-byte master keys (since otherwise HKDF-Extract
> using CMAC-AES would produce a pseudorandom key only 16 bytes long which
> is only enough for AES-128, not AES-256).
> 
> HKDF-SHA512 can require more "crypto work" per key derivation when
> compared to the current KDF.  However, later in this series, we'll start
> caching the HMAC transform for each master key, which will actually make
> the real-world performance about the same or even significantly better
> than the AES-based KDF as currently implemented.  Also, each KDF can
> actually be executed on the order of 1 million times per second, so KDF
> performance probably isn't actually the bottleneck in practice anyway.
> 
> References:
>   [1] Krawczyk (2010). "Cryptographic Extraction and Key Derivation: The
>       HKDF Scheme".  https://eprint.iacr.org/2010/264.pdf
> 
>   [2] RFC 5869. "HMAC-based Extract-and-Expand Key Derivation Function
>       (HKDF)".  https://tools.ietf.org/html/rfc5869
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/crypto/Kconfig           |   2 +
>  fs/crypto/fscrypt_private.h |  14 ++
>  fs/crypto/keyinfo.c         | 485 +++++++++++++++++++++++++++++++++++---------
>  3 files changed, 405 insertions(+), 96 deletions(-)
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index 02b7d91c9231..bbd4e38b293c 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -8,6 +8,8 @@ config FS_ENCRYPTION
>  	select CRYPTO_CTS
>  	select CRYPTO_CTR
>  	select CRYPTO_SHA256
> +	select CRYPTO_SHA512
> +	select CRYPTO_HMAC
>  	select KEYS
>  	help
>  	  Enable encryption of files and directories.  This
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 5470aac82cab..095e7c16483a 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -86,6 +86,14 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
>  	return size >= 1 && size == fscrypt_context_size(ctx);
>  }
>  
> +/*
> + * fscrypt_master_key - an in-use master key
> + */
> +struct fscrypt_master_key {
> +	struct crypto_shash	*mk_hmac;
> +	unsigned int		mk_size;
> +};
> +
>  /*
>   * fscrypt_info - the "encryption key" for an inode
>   *
> @@ -99,6 +107,12 @@ struct fscrypt_info {
>  	struct crypto_skcipher *ci_ctfm;
>  	struct crypto_cipher *ci_essiv_tfm;
>  
> +	/*
> +	 * The master key with which this inode was "unlocked"
> +	 * (only set for inodes that use a v2+ encryption policy)
> +	 */
> +	struct fscrypt_master_key *ci_master_key;
> +
>  	/*
>  	 * Cached fields from the fscrypt_context needed for encryption policy
>  	 * inheritance and enforcement
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 5591fd24e4b2..7ed1a7fb1308 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -6,17 +6,312 @@
>   * This contains encryption key functions.
>   *
>   * Written by Michael Halcrow, Ildar Muslukhov, and Uday Savagaonkar, 2015.
> + * HKDF support added by Eric Biggers, 2017.
> + *
> + * The implementation and usage of HKDF should conform to RFC-5869 ("HMAC-based
> + * Extract-and-Expand Key Derivation Function").
>   */
>  
>  #include <keys/user-type.h>
>  #include <linux/scatterlist.h>
>  #include <linux/ratelimit.h>
>  #include <crypto/aes.h>
> +#include <crypto/hash.h>
>  #include <crypto/sha.h>
>  #include "fscrypt_private.h"
>  
>  static struct crypto_shash *essiv_hash_tfm;
>  
> +/*
> + * Any unkeyed cryptographic hash algorithm can be used with HKDF, but we use
> + * SHA-512 because it is reasonably secure and efficient; and since it produces
> + * a 64-byte digest, deriving an AES-256-XTS key preserves all 64 bytes of
> + * entropy from the master key and requires only one iteration of HKDF-Expand.
> + */
> +#define HKDF_HMAC_ALG		"hmac(sha512)"
> +#define HKDF_HASHLEN		SHA512_DIGEST_SIZE
> +
> +/*
> + * The list of contexts in which we use HKDF to derive additional keys from a
> + * master key.  The values in this list are used as the first byte of the
> + * application-specific info string to guarantee that info strings are never
> + * repeated between contexts.
> + *
> + * Keys derived with different info strings are cryptographically isolated from
> + * each other --- knowledge of one derived key doesn't reveal any others.
> + */
> +#define HKDF_CONTEXT_PER_FILE_KEY	1
> +
> +/*
> + * HKDF consists of two steps:
> + *
> + * 1. HKDF-Extract: extract a fixed-length pseudorandom key from the
> + *    input keying material and optional salt.
> + * 2. HDKF-Expand: expand the pseudorandom key into output keying material of
> + *    any length, parameterized by an application-specific info string.
> + *
> + * HKDF-Extract can be skipped if the input is already a good pseudorandom key
> + * that is at least as long as the hash.  While the fscrypt master keys should
> + * already be good pseudorandom keys, when using encryption algorithms that use
> + * short keys (e.g. AES-128-CBC) we'd like to permit the master key to be
> + * shorter than HKDF_HASHLEN bytes.  Thus, we still must do HKDF-Extract.
> + *
> + * Ideally, HKDF-Extract would be passed a random salt for each distinct input
> + * key.  Details about the advantages of a random salt can be found in the HKDF
> + * paper (Krawczyk, 2010; "Cryptographic Extraction and Key Derivation: The HKDF
> + * Scheme").  However, we do not have the ability to store a salt on a
> + * per-master-key basis.  Thus, we have to use a fixed salt.  This is sufficient
> + * as long as the master keys are already pseudorandom and are long enough to
> + * make dictionary attacks infeasible.  This should be the case if userspace
> + * used a cryptographically secure random number generator, e.g. /dev/urandom,

Modulo entropy gathered since boot.

> + * to generate the master keys.
> + *
> + * For the fixed salt we use "fscrypt_hkdf_salt" rather than default of all 0's
> + * defined by RFC-5869.  This is only to be slightly more robust against
> + * userspace (unwisely) reusing the master keys for different purposes.
> + * Logically, it's more likely that the keys would be passed to unsalted
> + * HKDF-SHA512 than specifically to "fscrypt_hkdf_salt"-salted HKDF-SHA512.
> + * (Of course, a random salt would be better for this purpose.)
> + */
> +
> +#define HKDF_SALT		"fscrypt_hkdf_salt"
> +#define HKDF_SALT_LEN		(sizeof(HKDF_SALT) - 1)
> +
> +/*
> + * HKDF-Extract (RFC-5869 section 2.2).  This extracts a pseudorandom key 'prk'
> + * from the input key material 'ikm' and a salt.  (See explanation above for why
> + * we use a fixed salt.)
> + */
> +static int hkdf_extract(struct crypto_shash *hmac,
> +			const u8 *ikm, unsigned int ikmlen,
> +			u8 prk[HKDF_HASHLEN])
> +{
> +	SHASH_DESC_ON_STACK(desc, hmac);
> +	int err;
> +
> +	desc->tfm = hmac;
> +	desc->flags = 0;
> +
> +	err = crypto_shash_setkey(hmac, HKDF_SALT, HKDF_SALT_LEN);
> +	if (err)
> +		goto out;
> +
> +	err = crypto_shash_digest(desc, ikm, ikmlen, prk);
> +out:
> +	shash_desc_zero(desc);
> +	return err;
> +}
> +
> +/*
> + * HKDF-Expand (RFC-5869 section 2.3).  This expands the pseudorandom key, which
> + * has already been keyed into 'hmac', into 'okmlen' bytes of output keying
> + * material, parameterized by the application-specific information string of
> + * 'info' prefixed with the 'context' byte.  ('context' isn't part of the HKDF
> + * specification; it's just a prefix we add to our application-specific info
> + * strings to guarantee that we don't accidentally repeat an info string when
> + * using HKDF for different purposes.)
> + */
> +static int hkdf_expand(struct crypto_shash *hmac, u8 context,
> +		       const u8 *info, unsigned int infolen,
> +		       u8 *okm, unsigned int okmlen)
> +{
> +	SHASH_DESC_ON_STACK(desc, hmac);
> +	int err;
> +	const u8 *prev = NULL;
> +	unsigned int i;
> +	u8 counter = 1;
> +	u8 tmp[HKDF_HASHLEN];
> +
> +	desc->tfm = hmac;
> +	desc->flags = 0;
> +
> +	if (unlikely(okmlen > 255 * HKDF_HASHLEN))
> +		return -EINVAL;
> +
> +	for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
> +
> +		err = crypto_shash_init(desc);
> +		if (err)
> +			goto out;
> +
> +		if (prev) {
> +			err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
> +			if (err)
> +				goto out;
> +		}
> +
> +		err = crypto_shash_update(desc, &context, 1);

One potential shortcut would be to just increment context on each
iteration rather than maintain the counter.

> +		if (err)
> +			goto out;
> +
> +		err = crypto_shash_update(desc, info, infolen);
> +		if (err)
> +			goto out;
> +
> +		if (okmlen - i < HKDF_HASHLEN) {
> +			err = crypto_shash_finup(desc, &counter, 1, tmp);
> +			if (err)
> +				goto out;
> +			memcpy(&okm[i], tmp, okmlen - i);
> +			memzero_explicit(tmp, sizeof(tmp));
> +		} else {
> +			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
> +			if (err)
> +				goto out;
> +		}
> +		counter++;
> +		prev = &okm[i];
> +	}
> +	err = 0;
> +out:
> +	shash_desc_zero(desc);
> +	return err;
> +}
> +
> +static void put_master_key(struct fscrypt_master_key *k)
> +{
> +	if (!k)
> +		return;
> +
> +	crypto_free_shash(k->mk_hmac);
> +	kzfree(k);
> +}
> +
> +/*
> + * Allocate a fscrypt_master_key, given the keyring key payload.  This includes
> + * allocating and keying an HMAC transform so that we can efficiently derive

a HMAC?

an fscrypt_master_key?

> + * the per-inode encryption keys with HKDF-Expand later.
> + */
> +static struct fscrypt_master_key *
> +alloc_master_key(const struct fscrypt_key *payload)
> +{
> +	struct fscrypt_master_key *k;
> +	int err;
> +	u8 prk[HKDF_HASHLEN];
> +
> +	k = kzalloc(sizeof(*k), GFP_NOFS);
> +	if (!k)
> +		return ERR_PTR(-ENOMEM);
> +	k->mk_size = payload->size;
> +
> +	k->mk_hmac = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0);
> +	if (IS_ERR(k->mk_hmac)) {
> +		err = PTR_ERR(k->mk_hmac);
> +		k->mk_hmac = NULL;
> +		pr_warn("fscrypt: error allocating " HKDF_HMAC_ALG ": %d\n",
> +			err);
> +		goto fail;
> +	}
> +
> +	BUG_ON(crypto_shash_digestsize(k->mk_hmac) != sizeof(prk));
> +
> +	err = hkdf_extract(k->mk_hmac, payload->raw, payload->size, prk);
> +	if (err)
> +		goto fail;
> +
> +	err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
> +	if (err)
> +		goto fail;

Why not memzero prk?

> +out:
> +	memzero_explicit(prk, sizeof(prk));
> +	return k;
> +
> +fail:
> +	put_master_key(k);
> +	k = ERR_PTR(err);
> +	goto out;
> +}
> +
> +static void release_keyring_key(struct key *keyring_key)
> +{
> +	up_read(&keyring_key->sem);
> +	key_put(keyring_key);
> +}
> +
> +/*
> + * Find, lock, and validate the master key with the keyring description
> + * prefix:descriptor.  It must be released with release_keyring_key() later.
> + */
> +static struct key *
> +find_and_lock_keyring_key(const char *prefix,
> +			  const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
> +			  unsigned int min_keysize,
> +			  const struct fscrypt_key **payload_ret)
> +{
> +	char *description;
> +	struct key *keyring_key;
> +	const struct user_key_payload *ukp;
> +	const struct fscrypt_key *payload;
> +
> +	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> +				FS_KEY_DESCRIPTOR_SIZE, descriptor);
> +	if (!description)
> +		return ERR_PTR(-ENOMEM);
> +
> +	keyring_key = request_key(&key_type_logon, description, NULL);
> +	if (IS_ERR(keyring_key))
> +		goto out;
> +
> +	down_read(&keyring_key->sem);
> +	ukp = user_key_payload_locked(keyring_key);
> +	payload = (const struct fscrypt_key *)ukp->data;
> +
> +	if (ukp->datalen != sizeof(struct fscrypt_key) ||
> +	    payload->size == 0 || payload->size > FS_MAX_KEY_SIZE) {
> +		pr_warn_ratelimited("fscrypt: key '%s' has invalid payload\n",
> +				    description);
> +		goto invalid;
> +	}
> +
> +	/*
> +	 * With the legacy AES-based KDF the master key must be at least as long
> +	 * as the derived key.  With HKDF we could accept a shorter master key;
> +	 * however, that would mean the derived key wouldn't contain as much
> +	 * entropy as intended.  So don't allow it in either case.
> +	 */
> +	if (payload->size < min_keysize) {
> +		pr_warn_ratelimited("fscrypt: key '%s' is too short (got %u bytes, wanted %u+ bytes)\n",
> +				    description, payload->size, min_keysize);
> +		goto invalid;
> +	}
> +
> +	*payload_ret = payload;
> +out:
> +	kfree(description);
> +	return keyring_key;
> +
> +invalid:
> +	release_keyring_key(keyring_key);
> +	keyring_key = ERR_PTR(-ENOKEY);
> +	goto out;
> +}
> +
> +static struct fscrypt_master_key *
> +load_master_key_from_keyring(const struct inode *inode,
> +			     const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
> +			     unsigned int min_keysize)
> +{
> +	struct key *keyring_key;
> +	const struct fscrypt_key *payload;
> +	struct fscrypt_master_key *master_key;
> +
> +	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX, descriptor,
> +						min_keysize, &payload);
> +	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
> +		keyring_key = find_and_lock_keyring_key(
> +					inode->i_sb->s_cop->key_prefix,
> +					descriptor, min_keysize, &payload);
> +	}
> +	if (IS_ERR(keyring_key))
> +		return ERR_CAST(keyring_key);
> +
> +	master_key = alloc_master_key(payload);
> +
> +	release_keyring_key(keyring_key);
> +
> +	return master_key;
> +}
> +
>  static void derive_crypt_complete(struct crypto_async_request *req, int rc)
>  {
>  	struct fscrypt_completion_result *ecr = req->data;
> @@ -28,107 +323,100 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
>  	complete(&ecr->completion);
>  }
>  
> -/**
> - * derive_key_aes() - Derive a key using AES-128-ECB
> - * @deriving_key: Encryption key used for derivation.
> - * @source_key:   Source key to which to apply derivation.
> - * @derived_raw_key:  Derived raw key.
> - *
> - * Return: Zero on success; non-zero otherwise.
> +/*
> + * Legacy key derivation function.  This generates the derived key by encrypting
> + * the master key with AES-128-ECB using the nonce as the AES key.  This
> + * provides a unique derived key for each inode, but it's nonstandard, isn't
> + * very extensible, and has the weakness that it's trivially reversible: an
> + * attacker who compromises a derived key, e.g. with a side channel attack, can
> + * "decrypt" it to get back to the master key, then derive any other key.
>   */
> -static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
> -				const struct fscrypt_key *source_key,
> -				u8 derived_raw_key[FS_MAX_KEY_SIZE])
> +static int derive_key_aes(const struct fscrypt_key *master_key,
> +			  const struct fscrypt_context *ctx,
> +			  u8 *derived_key, unsigned int derived_keysize)
>  {
> -	int res = 0;
> +	int err;
>  	struct skcipher_request *req = NULL;
>  	DECLARE_FS_COMPLETION_RESULT(ecr);
>  	struct scatterlist src_sg, dst_sg;
> -	struct crypto_skcipher *tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
> +	struct crypto_skcipher *tfm;
> +
> +	tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
> +	if (IS_ERR(tfm))
> +		return PTR_ERR(tfm);
>  
> -	if (IS_ERR(tfm)) {
> -		res = PTR_ERR(tfm);
> -		tfm = NULL;
> -		goto out;
> -	}
>  	crypto_skcipher_set_flags(tfm, CRYPTO_TFM_REQ_WEAK_KEY);
>  	req = skcipher_request_alloc(tfm, GFP_NOFS);
>  	if (!req) {
> -		res = -ENOMEM;
> +		err = -ENOMEM;
>  		goto out;
>  	}
>  	skcipher_request_set_callback(req,
>  			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>  			derive_crypt_complete, &ecr);
> -	res = crypto_skcipher_setkey(tfm, deriving_key,
> -					FS_AES_128_ECB_KEY_SIZE);
> -	if (res < 0)
> +
> +	BUILD_BUG_ON(sizeof(ctx->nonce) != FS_AES_128_ECB_KEY_SIZE);
> +	err = crypto_skcipher_setkey(tfm, ctx->nonce, sizeof(ctx->nonce));
> +	if (err)
>  		goto out;
>  
> -	sg_init_one(&src_sg, source_key->raw, source_key->size);
> -	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
> -	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
> +	sg_init_one(&src_sg, master_key->raw, derived_keysize);
> +	sg_init_one(&dst_sg, derived_key, derived_keysize);
> +	skcipher_request_set_crypt(req, &src_sg, &dst_sg, derived_keysize,
>  				   NULL);
> -	res = crypto_skcipher_encrypt(req);
> -	if (res == -EINPROGRESS || res == -EBUSY) {
> +	err = crypto_skcipher_encrypt(req);
> +	if (err == -EINPROGRESS || err == -EBUSY) {
>  		wait_for_completion(&ecr.completion);
> -		res = ecr.res;
> +		err = ecr.res;
>  	}
>  out:
>  	skcipher_request_free(req);
>  	crypto_free_skcipher(tfm);
> -	return res;
> +	return err;
>  }
>  
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> -			struct fscrypt_context *ctx, u8 *raw_key,
> -			const char *prefix, int min_keysize)
> +/*
> + * HKDF-based key derivation function.  This uses HKDF-SHA512 to derive a unique
> + * encryption key for each inode, using the inode's nonce prefixed with a
> + * context byte as the application-specific information string.  This is more
> + * flexible than the legacy AES-based KDF and has the advantage that it's
> + * non-reversible: an attacker who compromises a derived key cannot calculate
> + * the master key or any other derived keys.
> + */
> +static int derive_key_hkdf(const struct fscrypt_master_key *master_key,
> +			   const struct fscrypt_context *ctx,
> +			   u8 *derived_key, unsigned int derived_keysize)
>  {
> -	char *description;
> -	struct key *keyring_key;
> -	struct fscrypt_key *master_key;
> -	const struct user_key_payload *ukp;
> -	int res;
> +	return hkdf_expand(master_key->mk_hmac, HKDF_CONTEXT_PER_FILE_KEY,
> +			   ctx->nonce, sizeof(ctx->nonce),
> +			   derived_key, derived_keysize);
> +}
>  
> -	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> -				FS_KEY_DESCRIPTOR_SIZE,
> -				ctx->master_key_descriptor);
> -	if (!description)
> -		return -ENOMEM;
> +static int find_and_derive_key_v1(const struct inode *inode,
> +				  const struct fscrypt_context *ctx,
> +				  u8 *derived_key, unsigned int derived_keysize)
> +{
> +	struct key *keyring_key;
> +	const struct fscrypt_key *payload;
> +	int err;
>  
> -	keyring_key = request_key(&key_type_logon, description, NULL);
> -	kfree(description);
> +	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX,
> +						ctx->master_key_descriptor,
> +						derived_keysize, &payload);
> +	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
> +		keyring_key = find_and_lock_keyring_key(
> +					inode->i_sb->s_cop->key_prefix,
> +					ctx->master_key_descriptor,
> +					derived_keysize, &payload);
> +	}
>  	if (IS_ERR(keyring_key))
>  		return PTR_ERR(keyring_key);
> -	down_read(&keyring_key->sem);
>  
> -	if (keyring_key->type != &key_type_logon) {
> -		printk_once(KERN_WARNING
> -				"%s: key type must be logon\n", __func__);
> -		res = -ENOKEY;
> -		goto out;
> -	}
> -	ukp = user_key_payload_locked(keyring_key);
> -	if (ukp->datalen != sizeof(struct fscrypt_key)) {
> -		res = -EINVAL;
> -		goto out;
> -	}
> -	master_key = (struct fscrypt_key *)ukp->data;
> -	BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
> -
> -	if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> -	    || master_key->size % AES_BLOCK_SIZE != 0) {
> -		printk_once(KERN_WARNING
> -				"%s: key size incorrect: %d\n",
> -				__func__, master_key->size);
> -		res = -ENOKEY;
> -		goto out;
> -	}
> -	res = derive_key_aes(ctx->nonce, master_key, raw_key);
> -out:
> -	up_read(&keyring_key->sem);
> -	key_put(keyring_key);
> -	return res;
> +	err = derive_key_aes(payload, ctx, derived_key, derived_keysize);
> +
> +	release_keyring_key(keyring_key);
> +
> +	return err;
>  }
>  
>  static const struct {
> @@ -179,6 +467,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  
>  	crypto_free_skcipher(ci->ci_ctfm);
>  	crypto_free_cipher(ci->ci_essiv_tfm);
> +	put_master_key(ci->ci_master_key);
>  	kmem_cache_free(fscrypt_info_cachep, ci);
>  }
>  
> @@ -254,8 +543,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	struct fscrypt_context ctx;
>  	struct crypto_skcipher *ctfm;
>  	const char *cipher_str;
> -	int keysize;
> -	u8 *raw_key = NULL;
> +	unsigned int derived_keysize;
> +	u8 *derived_key = NULL;
>  	int res;
>  
>  	if (inode->i_crypt_info)
> @@ -296,33 +585,40 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	memcpy(crypt_info->ci_master_key_descriptor, ctx.master_key_descriptor,
>  	       FS_KEY_DESCRIPTOR_SIZE);
>  
> -	res = determine_cipher_type(crypt_info, inode, &cipher_str, &keysize);
> +	res = determine_cipher_type(crypt_info, inode, &cipher_str,
> +				    &derived_keysize);
>  	if (res)
>  		goto out;
>  
>  	/*
> -	 * This cannot be a stack buffer because it is passed to the scatterlist
> -	 * crypto API as part of key derivation.
> +	 * This cannot be a stack buffer because it may be passed to the
> +	 * scatterlist crypto API during key derivation.
>  	 */
>  	res = -ENOMEM;
> -	raw_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
> -	if (!raw_key)
> +	derived_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
> +	if (!derived_key)
>  		goto out;
>  
> -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> -				keysize);
> -	if (res && inode->i_sb->s_cop->key_prefix) {
> -		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> -					     inode->i_sb->s_cop->key_prefix,
> -					     keysize);
> -		if (res2) {
> -			if (res2 == -ENOKEY)
> -				res = -ENOKEY;
> +	if (ctx.version == FSCRYPT_CONTEXT_V1) {
> +		res = find_and_derive_key_v1(inode, &ctx, derived_key,
> +					     derived_keysize);

Why not make this consistent with the else clause, i.e. doing
load_master_key_from_keyring() followed by derive_key_v1()?

> +	} else {
> +		crypt_info->ci_master_key =
> +			load_master_key_from_keyring(inode,
> +						     ctx.master_key_descriptor,
> +						     derived_keysize);
> +		if (IS_ERR(crypt_info->ci_master_key)) {
> +			res = PTR_ERR(crypt_info->ci_master_key);
> +			crypt_info->ci_master_key = NULL;
>  			goto out;
>  		}
> -	} else if (res) {
> -		goto out;
> +
> +		res = derive_key_hkdf(crypt_info->ci_master_key, &ctx,
> +				      derived_key, derived_keysize);
>  	}
> +	if (res)
> +		goto out;
> +
>  	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
>  	if (!ctfm || IS_ERR(ctfm)) {
>  		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
> @@ -333,17 +629,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	crypt_info->ci_ctfm = ctfm;
>  	crypto_skcipher_clear_flags(ctfm, ~0);
>  	crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_REQ_WEAK_KEY);
> -	/*
> -	 * if the provided key is longer than keysize, we use the first
> -	 * keysize bytes of the derived key only
> -	 */
> -	res = crypto_skcipher_setkey(ctfm, raw_key, keysize);
> +	res = crypto_skcipher_setkey(ctfm, derived_key, derived_keysize);
>  	if (res)
>  		goto out;
>  
>  	if (S_ISREG(inode->i_mode) &&
>  	    crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> -		res = init_essiv_generator(crypt_info, raw_key, keysize);
> +		res = init_essiv_generator(crypt_info, derived_key,
> +					   derived_keysize);
>  		if (res) {
>  			pr_debug("%s: error %d (inode %lu) allocating essiv tfm\n",
>  				 __func__, res, inode->i_ino);
> @@ -356,7 +649,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	if (res == -ENOKEY)
>  		res = 0;
>  	put_crypt_info(crypt_info);
> -	kzfree(raw_key);
> +	kzfree(derived_key);
>  	return res;
>  }
>  EXPORT_SYMBOL(fscrypt_get_encryption_info);
> -- 
> 2.13.2.932.g7449e964c-goog
> 

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Michael Halcrow]

On Fri, Jul 14, 2017 at 09:24:40AM -0700, Michael Halcrow wrote:
> On Wed, Jul 12, 2017 at 02:00:32PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > By design, the keys which userspace provides in the keyring are not used
> > to encrypt data directly.  Instead, a KDF (Key Derivation Function) is
> > used to derive a unique encryption key for each inode, given a "master"
> > key and a nonce.  The current KDF encrypts the master key with
> > AES-128-ECB using the nonce as the AES key.  This KDF is ad-hoc and is
> > not specified in any standard.  While it does generate unique derived
> > keys with sufficient entropy, it has several disadvantages:
> > 
> > - It's reversible: an attacker who compromises a derived key, e.g. using
> >   a side channel attack, can "decrypt" it to get back to the master key.
> > 
> > - It's not very extensible because it cannot easily be used to derive
> >   other key material that may be needed and it ties the length of the
> >   derived key closely to the length of the master key.
> > 
> > - It doesn't evenly distribute the entropy from the master key.  For
> >   example, the first 16 bytes of each derived key depend only on the
> >   first 16 bytes of the master key.
> > 
> > - It uses a public value as an AES key, which is unusual.  Ciphers are
> >   rarely evaluated under a threat model where the keys are public and
> >   the messages are secret.
> > 
> > Solve all these problems for v2 encryption policies by changing the KDF
> > to HKDF with SHA-512 as the underlying hash function.  To derive each
> > inode's encryption key, HKDF is executed with the master key as the
> > input key material, a fixed salt, and the per-inode nonce prefixed with
> > a context byte as the application-specific information string.  Unlike
> > the current KDF, HKDF has been formally published and standardized
> > [1][2], is nonreversible, can be used to derive any number and length of
> > secret and/or non-secret keys, and evenly distributes the entropy from
> > the master key (excepting limits inherent to not using a random salt).
> > 
> > Note that this introduces a dependency on the security and
> > implementation of SHA-512, whereas before we were using only AES for
> > both key derivation and encryption.  However, by using HMAC rather than
> > the hash function directly, HKDF is designed to remain secure even if
> > various classes of attacks, e.g. collision attacks, are found against
> > the underlying unkeyed hash function.  Even HMAC-MD5 is still considered
> > secure in practice, despite MD5 itself having been heavily compromised.
> > 
> > We *could* avoid introducing a hash function by instantiating
> > HKDF-Expand with CMAC-AES256 as the pseudorandom function rather than
> > HMAC-SHA512.  This would work; however, the HKDF specification doesn't
> > explicitly allow a non-HMAC pseudorandom function, so it would be less
> > standard.  It would also require skipping HKDF-Extract and changing the
> > API to accept only 32-byte master keys (since otherwise HKDF-Extract
> > using CMAC-AES would produce a pseudorandom key only 16 bytes long which
> > is only enough for AES-128, not AES-256).
> > 
> > HKDF-SHA512 can require more "crypto work" per key derivation when
> > compared to the current KDF.  However, later in this series, we'll start
> > caching the HMAC transform for each master key, which will actually make
> > the real-world performance about the same or even significantly better
> > than the AES-based KDF as currently implemented.  Also, each KDF can
> > actually be executed on the order of 1 million times per second, so KDF
> > performance probably isn't actually the bottleneck in practice anyway.
> > 
> > References:
> >   [1] Krawczyk (2010). "Cryptographic Extraction and Key Derivation: The
> >       HKDF Scheme".  https://eprint.iacr.org/2010/264.pdf
> > 
> >   [2] RFC 5869. "HMAC-based Extract-and-Expand Key Derivation Function
> >       (HKDF)".  https://tools.ietf.org/html/rfc5869
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  fs/crypto/Kconfig           |   2 +
> >  fs/crypto/fscrypt_private.h |  14 ++
> >  fs/crypto/keyinfo.c         | 485 +++++++++++++++++++++++++++++++++++---------
> >  3 files changed, 405 insertions(+), 96 deletions(-)
> > 
> > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> > index 02b7d91c9231..bbd4e38b293c 100644
> > --- a/fs/crypto/Kconfig
> > +++ b/fs/crypto/Kconfig
> > @@ -8,6 +8,8 @@ config FS_ENCRYPTION
> >  	select CRYPTO_CTS
> >  	select CRYPTO_CTR
> >  	select CRYPTO_SHA256
> > +	select CRYPTO_SHA512
> > +	select CRYPTO_HMAC
> >  	select KEYS
> >  	help
> >  	  Enable encryption of files and directories.  This
> > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> > index 5470aac82cab..095e7c16483a 100644
> > --- a/fs/crypto/fscrypt_private.h
> > +++ b/fs/crypto/fscrypt_private.h
> > @@ -86,6 +86,14 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
> >  	return size >= 1 && size == fscrypt_context_size(ctx);
> >  }
> >  
> > +/*
> > + * fscrypt_master_key - an in-use master key
> > + */
> > +struct fscrypt_master_key {
> > +	struct crypto_shash	*mk_hmac;
> > +	unsigned int		mk_size;
> > +};
> > +
> >  /*
> >   * fscrypt_info - the "encryption key" for an inode
> >   *
> > @@ -99,6 +107,12 @@ struct fscrypt_info {
> >  	struct crypto_skcipher *ci_ctfm;
> >  	struct crypto_cipher *ci_essiv_tfm;
> >  
> > +	/*
> > +	 * The master key with which this inode was "unlocked"
> > +	 * (only set for inodes that use a v2+ encryption policy)
> > +	 */
> > +	struct fscrypt_master_key *ci_master_key;
> > +
> >  	/*
> >  	 * Cached fields from the fscrypt_context needed for encryption policy
> >  	 * inheritance and enforcement
> > diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> > index 5591fd24e4b2..7ed1a7fb1308 100644
> > --- a/fs/crypto/keyinfo.c
> > +++ b/fs/crypto/keyinfo.c
> > @@ -6,17 +6,312 @@
> >   * This contains encryption key functions.
> >   *
> >   * Written by Michael Halcrow, Ildar Muslukhov, and Uday Savagaonkar, 2015.
> > + * HKDF support added by Eric Biggers, 2017.
> > + *
> > + * The implementation and usage of HKDF should conform to RFC-5869 ("HMAC-based
> > + * Extract-and-Expand Key Derivation Function").
> >   */
> >  
> >  #include <keys/user-type.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/ratelimit.h>
> >  #include <crypto/aes.h>
> > +#include <crypto/hash.h>
> >  #include <crypto/sha.h>
> >  #include "fscrypt_private.h"
> >  
> >  static struct crypto_shash *essiv_hash_tfm;
> >  
> > +/*
> > + * Any unkeyed cryptographic hash algorithm can be used with HKDF, but we use
> > + * SHA-512 because it is reasonably secure and efficient; and since it produces
> > + * a 64-byte digest, deriving an AES-256-XTS key preserves all 64 bytes of
> > + * entropy from the master key and requires only one iteration of HKDF-Expand.
> > + */
> > +#define HKDF_HMAC_ALG		"hmac(sha512)"
> > +#define HKDF_HASHLEN		SHA512_DIGEST_SIZE
> > +
> > +/*
> > + * The list of contexts in which we use HKDF to derive additional keys from a
> > + * master key.  The values in this list are used as the first byte of the
> > + * application-specific info string to guarantee that info strings are never
> > + * repeated between contexts.
> > + *
> > + * Keys derived with different info strings are cryptographically isolated from
> > + * each other --- knowledge of one derived key doesn't reveal any others.
> > + */
> > +#define HKDF_CONTEXT_PER_FILE_KEY	1
> > +
> > +/*
> > + * HKDF consists of two steps:
> > + *
> > + * 1. HKDF-Extract: extract a fixed-length pseudorandom key from the
> > + *    input keying material and optional salt.
> > + * 2. HDKF-Expand: expand the pseudorandom key into output keying material of
> > + *    any length, parameterized by an application-specific info string.
> > + *
> > + * HKDF-Extract can be skipped if the input is already a good pseudorandom key
> > + * that is at least as long as the hash.  While the fscrypt master keys should
> > + * already be good pseudorandom keys, when using encryption algorithms that use
> > + * short keys (e.g. AES-128-CBC) we'd like to permit the master key to be
> > + * shorter than HKDF_HASHLEN bytes.  Thus, we still must do HKDF-Extract.
> > + *
> > + * Ideally, HKDF-Extract would be passed a random salt for each distinct input
> > + * key.  Details about the advantages of a random salt can be found in the HKDF
> > + * paper (Krawczyk, 2010; "Cryptographic Extraction and Key Derivation: The HKDF
> > + * Scheme").  However, we do not have the ability to store a salt on a
> > + * per-master-key basis.  Thus, we have to use a fixed salt.  This is sufficient
> > + * as long as the master keys are already pseudorandom and are long enough to
> > + * make dictionary attacks infeasible.  This should be the case if userspace
> > + * used a cryptographically secure random number generator, e.g. /dev/urandom,
> 
> Modulo entropy gathered since boot.
> 
> > + * to generate the master keys.
> > + *
> > + * For the fixed salt we use "fscrypt_hkdf_salt" rather than default of all 0's
> > + * defined by RFC-5869.  This is only to be slightly more robust against
> > + * userspace (unwisely) reusing the master keys for different purposes.
> > + * Logically, it's more likely that the keys would be passed to unsalted
> > + * HKDF-SHA512 than specifically to "fscrypt_hkdf_salt"-salted HKDF-SHA512.
> > + * (Of course, a random salt would be better for this purpose.)
> > + */
> > +
> > +#define HKDF_SALT		"fscrypt_hkdf_salt"
> > +#define HKDF_SALT_LEN		(sizeof(HKDF_SALT) - 1)
> > +
> > +/*
> > + * HKDF-Extract (RFC-5869 section 2.2).  This extracts a pseudorandom key 'prk'
> > + * from the input key material 'ikm' and a salt.  (See explanation above for why
> > + * we use a fixed salt.)
> > + */
> > +static int hkdf_extract(struct crypto_shash *hmac,
> > +			const u8 *ikm, unsigned int ikmlen,
> > +			u8 prk[HKDF_HASHLEN])
> > +{
> > +	SHASH_DESC_ON_STACK(desc, hmac);
> > +	int err;
> > +
> > +	desc->tfm = hmac;
> > +	desc->flags = 0;
> > +
> > +	err = crypto_shash_setkey(hmac, HKDF_SALT, HKDF_SALT_LEN);
> > +	if (err)
> > +		goto out;
> > +
> > +	err = crypto_shash_digest(desc, ikm, ikmlen, prk);
> > +out:
> > +	shash_desc_zero(desc);
> > +	return err;
> > +}
> > +
> > +/*
> > + * HKDF-Expand (RFC-5869 section 2.3).  This expands the pseudorandom key, which
> > + * has already been keyed into 'hmac', into 'okmlen' bytes of output keying
> > + * material, parameterized by the application-specific information string of
> > + * 'info' prefixed with the 'context' byte.  ('context' isn't part of the HKDF
> > + * specification; it's just a prefix we add to our application-specific info
> > + * strings to guarantee that we don't accidentally repeat an info string when
> > + * using HKDF for different purposes.)
> > + */
> > +static int hkdf_expand(struct crypto_shash *hmac, u8 context,
> > +		       const u8 *info, unsigned int infolen,
> > +		       u8 *okm, unsigned int okmlen)
> > +{
> > +	SHASH_DESC_ON_STACK(desc, hmac);
> > +	int err;
> > +	const u8 *prev = NULL;
> > +	unsigned int i;
> > +	u8 counter = 1;
> > +	u8 tmp[HKDF_HASHLEN];
> > +
> > +	desc->tfm = hmac;
> > +	desc->flags = 0;
> > +
> > +	if (unlikely(okmlen > 255 * HKDF_HASHLEN))
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
> > +
> > +		err = crypto_shash_init(desc);
> > +		if (err)
> > +			goto out;
> > +
> > +		if (prev) {
> > +			err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
> > +			if (err)
> > +				goto out;
> > +		}
> > +
> > +		err = crypto_shash_update(desc, &context, 1);
> 
> One potential shortcut would be to just increment context on each
> iteration rather than maintain the counter.
> 
> > +		if (err)
> > +			goto out;
> > +
> > +		err = crypto_shash_update(desc, info, infolen);
> > +		if (err)
> > +			goto out;
> > +
> > +		if (okmlen - i < HKDF_HASHLEN) {
> > +			err = crypto_shash_finup(desc, &counter, 1, tmp);
> > +			if (err)
> > +				goto out;
> > +			memcpy(&okm[i], tmp, okmlen - i);
> > +			memzero_explicit(tmp, sizeof(tmp));
> > +		} else {
> > +			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
> > +			if (err)
> > +				goto out;
> > +		}
> > +		counter++;
> > +		prev = &okm[i];
> > +	}
> > +	err = 0;
> > +out:
> > +	shash_desc_zero(desc);
> > +	return err;
> > +}
> > +
> > +static void put_master_key(struct fscrypt_master_key *k)
> > +{
> > +	if (!k)
> > +		return;
> > +
> > +	crypto_free_shash(k->mk_hmac);
> > +	kzfree(k);
> > +}
> > +
> > +/*
> > + * Allocate a fscrypt_master_key, given the keyring key payload.  This includes
> > + * allocating and keying an HMAC transform so that we can efficiently derive
> 
> a HMAC?
> 
> an fscrypt_master_key?
> 
> > + * the per-inode encryption keys with HKDF-Expand later.
> > + */
> > +static struct fscrypt_master_key *
> > +alloc_master_key(const struct fscrypt_key *payload)
> > +{
> > +	struct fscrypt_master_key *k;
> > +	int err;
> > +	u8 prk[HKDF_HASHLEN];
> > +
> > +	k = kzalloc(sizeof(*k), GFP_NOFS);
> > +	if (!k)
> > +		return ERR_PTR(-ENOMEM);
> > +	k->mk_size = payload->size;
> > +
> > +	k->mk_hmac = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0);
> > +	if (IS_ERR(k->mk_hmac)) {
> > +		err = PTR_ERR(k->mk_hmac);
> > +		k->mk_hmac = NULL;
> > +		pr_warn("fscrypt: error allocating " HKDF_HMAC_ALG ": %d\n",
> > +			err);
> > +		goto fail;
> > +	}
> > +
> > +	BUG_ON(crypto_shash_digestsize(k->mk_hmac) != sizeof(prk));
> > +
> > +	err = hkdf_extract(k->mk_hmac, payload->raw, payload->size, prk);
> > +	if (err)
> > +		goto fail;
> > +
> > +	err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
> > +	if (err)
> > +		goto fail;
> 
> Why not memzero prk?

Scratch that thought. My brain apparently doesn't always jump
backwards very well.

I might suggest reworking so that goto only moves forward, if that can
be done sanely.

> > +out:
> > +	memzero_explicit(prk, sizeof(prk));
> > +	return k;
> > +
> > +fail:
> > +	put_master_key(k);
> > +	k = ERR_PTR(err);
> > +	goto out;
> > +}
> > +
> > +static void release_keyring_key(struct key *keyring_key)
> > +{
> > +	up_read(&keyring_key->sem);
> > +	key_put(keyring_key);
> > +}
> > +
> > +/*
> > + * Find, lock, and validate the master key with the keyring description
> > + * prefix:descriptor.  It must be released with release_keyring_key() later.
> > + */
> > +static struct key *
> > +find_and_lock_keyring_key(const char *prefix,
> > +			  const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
> > +			  unsigned int min_keysize,
> > +			  const struct fscrypt_key **payload_ret)
> > +{
> > +	char *description;
> > +	struct key *keyring_key;
> > +	const struct user_key_payload *ukp;
> > +	const struct fscrypt_key *payload;
> > +
> > +	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> > +				FS_KEY_DESCRIPTOR_SIZE, descriptor);
> > +	if (!description)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	keyring_key = request_key(&key_type_logon, description, NULL);
> > +	if (IS_ERR(keyring_key))
> > +		goto out;
> > +
> > +	down_read(&keyring_key->sem);
> > +	ukp = user_key_payload_locked(keyring_key);
> > +	payload = (const struct fscrypt_key *)ukp->data;
> > +
> > +	if (ukp->datalen != sizeof(struct fscrypt_key) ||
> > +	    payload->size == 0 || payload->size > FS_MAX_KEY_SIZE) {
> > +		pr_warn_ratelimited("fscrypt: key '%s' has invalid payload\n",
> > +				    description);
> > +		goto invalid;
> > +	}
> > +
> > +	/*
> > +	 * With the legacy AES-based KDF the master key must be at least as long
> > +	 * as the derived key.  With HKDF we could accept a shorter master key;
> > +	 * however, that would mean the derived key wouldn't contain as much
> > +	 * entropy as intended.  So don't allow it in either case.
> > +	 */
> > +	if (payload->size < min_keysize) {
> > +		pr_warn_ratelimited("fscrypt: key '%s' is too short (got %u bytes, wanted %u+ bytes)\n",
> > +				    description, payload->size, min_keysize);
> > +		goto invalid;
> > +	}
> > +
> > +	*payload_ret = payload;
> > +out:
> > +	kfree(description);
> > +	return keyring_key;
> > +
> > +invalid:
> > +	release_keyring_key(keyring_key);
> > +	keyring_key = ERR_PTR(-ENOKEY);
> > +	goto out;
> > +}
> > +
> > +static struct fscrypt_master_key *
> > +load_master_key_from_keyring(const struct inode *inode,
> > +			     const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
> > +			     unsigned int min_keysize)
> > +{
> > +	struct key *keyring_key;
> > +	const struct fscrypt_key *payload;
> > +	struct fscrypt_master_key *master_key;
> > +
> > +	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX, descriptor,
> > +						min_keysize, &payload);
> > +	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
> > +		keyring_key = find_and_lock_keyring_key(
> > +					inode->i_sb->s_cop->key_prefix,
> > +					descriptor, min_keysize, &payload);
> > +	}
> > +	if (IS_ERR(keyring_key))
> > +		return ERR_CAST(keyring_key);
> > +
> > +	master_key = alloc_master_key(payload);
> > +
> > +	release_keyring_key(keyring_key);
> > +
> > +	return master_key;
> > +}
> > +
> >  static void derive_crypt_complete(struct crypto_async_request *req, int rc)
> >  {
> >  	struct fscrypt_completion_result *ecr = req->data;
> > @@ -28,107 +323,100 @@ static void derive_crypt_complete(struct crypto_async_request *req, int rc)
> >  	complete(&ecr->completion);
> >  }
> >  
> > -/**
> > - * derive_key_aes() - Derive a key using AES-128-ECB
> > - * @deriving_key: Encryption key used for derivation.
> > - * @source_key:   Source key to which to apply derivation.
> > - * @derived_raw_key:  Derived raw key.
> > - *
> > - * Return: Zero on success; non-zero otherwise.
> > +/*
> > + * Legacy key derivation function.  This generates the derived key by encrypting
> > + * the master key with AES-128-ECB using the nonce as the AES key.  This
> > + * provides a unique derived key for each inode, but it's nonstandard, isn't
> > + * very extensible, and has the weakness that it's trivially reversible: an
> > + * attacker who compromises a derived key, e.g. with a side channel attack, can
> > + * "decrypt" it to get back to the master key, then derive any other key.
> >   */
> > -static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE],
> > -				const struct fscrypt_key *source_key,
> > -				u8 derived_raw_key[FS_MAX_KEY_SIZE])
> > +static int derive_key_aes(const struct fscrypt_key *master_key,
> > +			  const struct fscrypt_context *ctx,
> > +			  u8 *derived_key, unsigned int derived_keysize)
> >  {
> > -	int res = 0;
> > +	int err;
> >  	struct skcipher_request *req = NULL;
> >  	DECLARE_FS_COMPLETION_RESULT(ecr);
> >  	struct scatterlist src_sg, dst_sg;
> > -	struct crypto_skcipher *tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
> > +	struct crypto_skcipher *tfm;
> > +
> > +	tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
> > +	if (IS_ERR(tfm))
> > +		return PTR_ERR(tfm);
> >  
> > -	if (IS_ERR(tfm)) {
> > -		res = PTR_ERR(tfm);
> > -		tfm = NULL;
> > -		goto out;
> > -	}
> >  	crypto_skcipher_set_flags(tfm, CRYPTO_TFM_REQ_WEAK_KEY);
> >  	req = skcipher_request_alloc(tfm, GFP_NOFS);
> >  	if (!req) {
> > -		res = -ENOMEM;
> > +		err = -ENOMEM;
> >  		goto out;
> >  	}
> >  	skcipher_request_set_callback(req,
> >  			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
> >  			derive_crypt_complete, &ecr);
> > -	res = crypto_skcipher_setkey(tfm, deriving_key,
> > -					FS_AES_128_ECB_KEY_SIZE);
> > -	if (res < 0)
> > +
> > +	BUILD_BUG_ON(sizeof(ctx->nonce) != FS_AES_128_ECB_KEY_SIZE);
> > +	err = crypto_skcipher_setkey(tfm, ctx->nonce, sizeof(ctx->nonce));
> > +	if (err)
> >  		goto out;
> >  
> > -	sg_init_one(&src_sg, source_key->raw, source_key->size);
> > -	sg_init_one(&dst_sg, derived_raw_key, source_key->size);
> > -	skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size,
> > +	sg_init_one(&src_sg, master_key->raw, derived_keysize);
> > +	sg_init_one(&dst_sg, derived_key, derived_keysize);
> > +	skcipher_request_set_crypt(req, &src_sg, &dst_sg, derived_keysize,
> >  				   NULL);
> > -	res = crypto_skcipher_encrypt(req);
> > -	if (res == -EINPROGRESS || res == -EBUSY) {
> > +	err = crypto_skcipher_encrypt(req);
> > +	if (err == -EINPROGRESS || err == -EBUSY) {
> >  		wait_for_completion(&ecr.completion);
> > -		res = ecr.res;
> > +		err = ecr.res;
> >  	}
> >  out:
> >  	skcipher_request_free(req);
> >  	crypto_free_skcipher(tfm);
> > -	return res;
> > +	return err;
> >  }
> >  
> > -static int validate_user_key(struct fscrypt_info *crypt_info,
> > -			struct fscrypt_context *ctx, u8 *raw_key,
> > -			const char *prefix, int min_keysize)
> > +/*
> > + * HKDF-based key derivation function.  This uses HKDF-SHA512 to derive a unique
> > + * encryption key for each inode, using the inode's nonce prefixed with a
> > + * context byte as the application-specific information string.  This is more
> > + * flexible than the legacy AES-based KDF and has the advantage that it's
> > + * non-reversible: an attacker who compromises a derived key cannot calculate
> > + * the master key or any other derived keys.
> > + */
> > +static int derive_key_hkdf(const struct fscrypt_master_key *master_key,
> > +			   const struct fscrypt_context *ctx,
> > +			   u8 *derived_key, unsigned int derived_keysize)
> >  {
> > -	char *description;
> > -	struct key *keyring_key;
> > -	struct fscrypt_key *master_key;
> > -	const struct user_key_payload *ukp;
> > -	int res;
> > +	return hkdf_expand(master_key->mk_hmac, HKDF_CONTEXT_PER_FILE_KEY,
> > +			   ctx->nonce, sizeof(ctx->nonce),
> > +			   derived_key, derived_keysize);
> > +}
> >  
> > -	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> > -				FS_KEY_DESCRIPTOR_SIZE,
> > -				ctx->master_key_descriptor);
> > -	if (!description)
> > -		return -ENOMEM;
> > +static int find_and_derive_key_v1(const struct inode *inode,
> > +				  const struct fscrypt_context *ctx,
> > +				  u8 *derived_key, unsigned int derived_keysize)
> > +{
> > +	struct key *keyring_key;
> > +	const struct fscrypt_key *payload;
> > +	int err;
> >  
> > -	keyring_key = request_key(&key_type_logon, description, NULL);
> > -	kfree(description);
> > +	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX,
> > +						ctx->master_key_descriptor,
> > +						derived_keysize, &payload);
> > +	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
> > +		keyring_key = find_and_lock_keyring_key(
> > +					inode->i_sb->s_cop->key_prefix,
> > +					ctx->master_key_descriptor,
> > +					derived_keysize, &payload);
> > +	}
> >  	if (IS_ERR(keyring_key))
> >  		return PTR_ERR(keyring_key);
> > -	down_read(&keyring_key->sem);
> >  
> > -	if (keyring_key->type != &key_type_logon) {
> > -		printk_once(KERN_WARNING
> > -				"%s: key type must be logon\n", __func__);
> > -		res = -ENOKEY;
> > -		goto out;
> > -	}
> > -	ukp = user_key_payload_locked(keyring_key);
> > -	if (ukp->datalen != sizeof(struct fscrypt_key)) {
> > -		res = -EINVAL;
> > -		goto out;
> > -	}
> > -	master_key = (struct fscrypt_key *)ukp->data;
> > -	BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
> > -
> > -	if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> > -	    || master_key->size % AES_BLOCK_SIZE != 0) {
> > -		printk_once(KERN_WARNING
> > -				"%s: key size incorrect: %d\n",
> > -				__func__, master_key->size);
> > -		res = -ENOKEY;
> > -		goto out;
> > -	}
> > -	res = derive_key_aes(ctx->nonce, master_key, raw_key);
> > -out:
> > -	up_read(&keyring_key->sem);
> > -	key_put(keyring_key);
> > -	return res;
> > +	err = derive_key_aes(payload, ctx, derived_key, derived_keysize);
> > +
> > +	release_keyring_key(keyring_key);
> > +
> > +	return err;
> >  }
> >  
> >  static const struct {
> > @@ -179,6 +467,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
> >  
> >  	crypto_free_skcipher(ci->ci_ctfm);
> >  	crypto_free_cipher(ci->ci_essiv_tfm);
> > +	put_master_key(ci->ci_master_key);
> >  	kmem_cache_free(fscrypt_info_cachep, ci);
> >  }
> >  
> > @@ -254,8 +543,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
> >  	struct fscrypt_context ctx;
> >  	struct crypto_skcipher *ctfm;
> >  	const char *cipher_str;
> > -	int keysize;
> > -	u8 *raw_key = NULL;
> > +	unsigned int derived_keysize;
> > +	u8 *derived_key = NULL;
> >  	int res;
> >  
> >  	if (inode->i_crypt_info)
> > @@ -296,33 +585,40 @@ int fscrypt_get_encryption_info(struct inode *inode)
> >  	memcpy(crypt_info->ci_master_key_descriptor, ctx.master_key_descriptor,
> >  	       FS_KEY_DESCRIPTOR_SIZE);
> >  
> > -	res = determine_cipher_type(crypt_info, inode, &cipher_str, &keysize);
> > +	res = determine_cipher_type(crypt_info, inode, &cipher_str,
> > +				    &derived_keysize);
> >  	if (res)
> >  		goto out;
> >  
> >  	/*
> > -	 * This cannot be a stack buffer because it is passed to the scatterlist
> > -	 * crypto API as part of key derivation.
> > +	 * This cannot be a stack buffer because it may be passed to the
> > +	 * scatterlist crypto API during key derivation.
> >  	 */
> >  	res = -ENOMEM;
> > -	raw_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
> > -	if (!raw_key)
> > +	derived_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
> > +	if (!derived_key)
> >  		goto out;
> >  
> > -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> > -				keysize);
> > -	if (res && inode->i_sb->s_cop->key_prefix) {
> > -		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> > -					     inode->i_sb->s_cop->key_prefix,
> > -					     keysize);
> > -		if (res2) {
> > -			if (res2 == -ENOKEY)
> > -				res = -ENOKEY;
> > +	if (ctx.version == FSCRYPT_CONTEXT_V1) {
> > +		res = find_and_derive_key_v1(inode, &ctx, derived_key,
> > +					     derived_keysize);
> 
> Why not make this consistent with the else clause, i.e. doing
> load_master_key_from_keyring() followed by derive_key_v1()?
> 
> > +	} else {
> > +		crypt_info->ci_master_key =
> > +			load_master_key_from_keyring(inode,
> > +						     ctx.master_key_descriptor,
> > +						     derived_keysize);
> > +		if (IS_ERR(crypt_info->ci_master_key)) {
> > +			res = PTR_ERR(crypt_info->ci_master_key);
> > +			crypt_info->ci_master_key = NULL;
> >  			goto out;
> >  		}
> > -	} else if (res) {
> > -		goto out;
> > +
> > +		res = derive_key_hkdf(crypt_info->ci_master_key, &ctx,
> > +				      derived_key, derived_keysize);
> >  	}
> > +	if (res)
> > +		goto out;
> > +
> >  	ctfm = crypto_alloc_skcipher(cipher_str, 0, 0);
> >  	if (!ctfm || IS_ERR(ctfm)) {
> >  		res = ctfm ? PTR_ERR(ctfm) : -ENOMEM;
> > @@ -333,17 +629,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
> >  	crypt_info->ci_ctfm = ctfm;
> >  	crypto_skcipher_clear_flags(ctfm, ~0);
> >  	crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_REQ_WEAK_KEY);
> > -	/*
> > -	 * if the provided key is longer than keysize, we use the first
> > -	 * keysize bytes of the derived key only
> > -	 */
> > -	res = crypto_skcipher_setkey(ctfm, raw_key, keysize);
> > +	res = crypto_skcipher_setkey(ctfm, derived_key, derived_keysize);
> >  	if (res)
> >  		goto out;
> >  
> >  	if (S_ISREG(inode->i_mode) &&
> >  	    crypt_info->ci_data_mode == FS_ENCRYPTION_MODE_AES_128_CBC) {
> > -		res = init_essiv_generator(crypt_info, raw_key, keysize);
> > +		res = init_essiv_generator(crypt_info, derived_key,
> > +					   derived_keysize);
> >  		if (res) {
> >  			pr_debug("%s: error %d (inode %lu) allocating essiv tfm\n",
> >  				 __func__, res, inode->i_ino);
> > @@ -356,7 +649,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
> >  	if (res == -ENOKEY)
> >  		res = 0;
> >  	put_crypt_info(crypt_info);
> > -	kzfree(raw_key);
> > +	kzfree(derived_key);
> >  	return res;
> >  }
> >  EXPORT_SYMBOL(fscrypt_get_encryption_info);
> > -- 
> > 2.13.2.932.g7449e964c-goog
> > 

Re: [PATCH 3/6] fscrypt: use HKDF-SHA512 to derive the per-inode encryption keys

[Eric Biggers]

On Fri, Jul 14, 2017 at 09:24:40AM -0700, Michael Halcrow wrote:
> > +static int hkdf_expand(struct crypto_shash *hmac, u8 context,
> > +		       const u8 *info, unsigned int infolen,
> > +		       u8 *okm, unsigned int okmlen)
> > +{
> > +	SHASH_DESC_ON_STACK(desc, hmac);
> > +	int err;
> > +	const u8 *prev = NULL;
> > +	unsigned int i;
> > +	u8 counter = 1;
> > +	u8 tmp[HKDF_HASHLEN];
> > +
> > +	desc->tfm = hmac;
> > +	desc->flags = 0;
> > +
> > +	if (unlikely(okmlen > 255 * HKDF_HASHLEN))
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
> > +
> > +		err = crypto_shash_init(desc);
> > +		if (err)
> > +			goto out;
> > +
> > +		if (prev) {
> > +			err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
> > +			if (err)
> > +				goto out;
> > +		}
> > +
> > +		err = crypto_shash_update(desc, &context, 1);
> 
> One potential shortcut would be to just increment context on each
> iteration rather than maintain the counter.
> 

That's not a good idea because then it wouldn't be standard HKDF, and it would
be relying on the "feedback" mode to keep the HMAC inputs unique which isn't
guaranteed to be sufficient.

> >  
> > -	res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> > -				keysize);
> > -	if (res && inode->i_sb->s_cop->key_prefix) {
> > -		int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> > -					     inode->i_sb->s_cop->key_prefix,
> > -					     keysize);
> > -		if (res2) {
> > -			if (res2 == -ENOKEY)
> > -				res = -ENOKEY;
> > +	if (ctx.version == FSCRYPT_CONTEXT_V1) {
> > +		res = find_and_derive_key_v1(inode, &ctx, derived_key,
> > +					     derived_keysize);
> 
> Why not make this consistent with the else clause, i.e. doing
> load_master_key_from_keyring() followed by derive_key_v1()?
> 

struct fscrypt_master_key contains the HMAC transform but not the raw master
key.  For the v1 key derivation we need the raw master key.  We could put it in
the fscrypt_master_key and then try to allow fscrypt_master_key's both with and
without HMAC transforms depending on the policy versions they are used for, but
there's no point in doing so currently.

Eric

[PATCH 4/6] fscrypt: verify that the correct master key was supplied

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, while a fscrypt master key is required to have a certain
description in the keyring, its payload is never verified to be correct.
While sufficient for well-behaved userspace, this is insecure in a
multi-user system where a user has been given only read-only access to
an encrypted file or directory.  Specifically, if an encrypted file or
directory does not yet have its key cached by the kernel, the first user
who accesses it can provide an arbitrary key in their own keyring, which
the kernel will then associate with the inode and use for read(),
write(), readdir(), etc. by other users as well.

Consequently, it's trivial for a user with *read-only* access to an
encrypted file or directory to make it appear as garbage to everyone.
Creative users might be able to accomplish more sophisticated attacks by
careful choice of the key, e.g. choosing a key causes certain bytes of
file contents to have certain values or that causes filenames containing
the '/' character to appear.

Solve the problem for v2 encryption policies by storing a "hash" of the
master encryption key in the encryption xattr and verifying it before
accepting the user-provided key.  We generate the "hash" using
HKDF-SHA512 by passing a distinct application-specific info string.
This produces a value which is cryptographically isolated and can be
stored in the clear without leaking any information about the master key
or any other derived keys (in a computational sense).  Reusing HKDF is
better than doing e.g. SHA-512(master_key) because it avoids passing the
same key into different cryptographic primitives.

We make the hash field 16 bytes long, as this should provide sufficient
collision and preimage resistance while not wasting too much space for
the encryption xattr.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h |  4 ++++
 fs/crypto/keyinfo.c         | 46 +++++++++++++++++++++++++++++++++++++
 fs/crypto/policy.c          | 55 ++++++++++++++++++++++++++++++++++++---------
 3 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 095e7c16483a..a7baeac92575 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -92,6 +92,7 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
 struct fscrypt_master_key {
 	struct crypto_shash	*mk_hmac;
 	unsigned int		mk_size;
+	u8			mk_hash[FSCRYPT_KEY_HASH_SIZE];
 };
 
 /*
@@ -155,6 +156,9 @@ extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
 
 /* keyinfo.c */
+extern int fscrypt_compute_key_hash(const struct inode *inode,
+				    const struct fscrypt_policy *policy,
+				    u8 hash[FSCRYPT_KEY_HASH_SIZE]);
 extern void __exit fscrypt_essiv_cleanup(void);
 
 #endif /* _FSCRYPT_PRIVATE_H */
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 7ed1a7fb1308..12a60eacf819 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -39,8 +39,11 @@ static struct crypto_shash *essiv_hash_tfm;
  *
  * Keys derived with different info strings are cryptographically isolated from
  * each other --- knowledge of one derived key doesn't reveal any others.
+ * (This property is particularly important for the derived key used as the
+ * "key hash", as that is stored in the clear.)
  */
 #define HKDF_CONTEXT_PER_FILE_KEY	1
+#define HKDF_CONTEXT_KEY_HASH		2
 
 /*
  * HKDF consists of two steps:
@@ -212,6 +215,12 @@ alloc_master_key(const struct fscrypt_key *payload)
 	err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
 	if (err)
 		goto fail;
+
+	/* Calculate the "key hash" */
+	err = hkdf_expand(k->mk_hmac, HKDF_CONTEXT_KEY_HASH, NULL, 0,
+			  k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
+	if (err)
+		goto fail;
 out:
 	memzero_explicit(prk, sizeof(prk));
 	return k;
@@ -537,6 +546,31 @@ void __exit fscrypt_essiv_cleanup(void)
 	crypto_free_shash(essiv_hash_tfm);
 }
 
+int fscrypt_compute_key_hash(const struct inode *inode,
+			     const struct fscrypt_policy *policy,
+			     u8 hash[FSCRYPT_KEY_HASH_SIZE])
+{
+	struct fscrypt_master_key *k;
+	unsigned int min_keysize;
+
+	/*
+	 * Require that the master key be long enough for both the
+	 * contents and filenames encryption modes.
+	 */
+	min_keysize =
+		max(available_modes[policy->contents_encryption_mode].keysize,
+		    available_modes[policy->filenames_encryption_mode].keysize);
+
+	k = load_master_key_from_keyring(inode, policy->master_key_descriptor,
+					 min_keysize);
+	if (IS_ERR(k))
+		return PTR_ERR(k);
+
+	memcpy(hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
+	put_master_key(k);
+	return 0;
+}
+
 int fscrypt_get_encryption_info(struct inode *inode)
 {
 	struct fscrypt_info *crypt_info;
@@ -613,6 +647,18 @@ int fscrypt_get_encryption_info(struct inode *inode)
 			goto out;
 		}
 
+		/*
+		 * Make sure the master key we found has the correct hash.
+		 * Buggy or malicious userspace may provide the wrong key.
+		 */
+		if (memcmp(crypt_info->ci_master_key->mk_hash, ctx.key_hash,
+			   FSCRYPT_KEY_HASH_SIZE)) {
+			pr_warn_ratelimited("fscrypt: wrong encryption key supplied for inode %lu\n",
+					    inode->i_ino);
+			res = -ENOKEY;
+			goto out;
+		}
+
 		res = derive_key_hkdf(crypt_info->ci_master_key, &ctx,
 				      derived_key, derived_keysize);
 	}
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 81c59f8e45c0..2934bc2bff4b 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -40,7 +40,8 @@ static u8 context_version_for_policy(const struct fscrypt_policy *policy)
  */
 static bool is_encryption_context_consistent_with_policy(
 				const struct fscrypt_context *ctx,
-				const struct fscrypt_policy *policy)
+				const struct fscrypt_policy *policy,
+				const u8 key_hash[FSCRYPT_KEY_HASH_SIZE])
 {
 	return (ctx->version == context_version_for_policy(policy)) &&
 		(memcmp(ctx->master_key_descriptor,
@@ -50,11 +51,14 @@ static bool is_encryption_context_consistent_with_policy(
 		(ctx->contents_encryption_mode ==
 		 policy->contents_encryption_mode) &&
 		(ctx->filenames_encryption_mode ==
-		 policy->filenames_encryption_mode);
+		 policy->filenames_encryption_mode) &&
+		(ctx->version == FSCRYPT_CONTEXT_V1 ||
+		 (memcmp(ctx->key_hash, key_hash, FSCRYPT_KEY_HASH_SIZE) == 0));
 }
 
 static int create_encryption_context_from_policy(struct inode *inode,
-				const struct fscrypt_policy *policy)
+				const struct fscrypt_policy *policy,
+				const u8 key_hash[FSCRYPT_KEY_HASH_SIZE])
 {
 	struct fscrypt_context ctx;
 
@@ -74,7 +78,7 @@ static int create_encryption_context_from_policy(struct inode *inode,
 	BUILD_BUG_ON(sizeof(ctx.nonce) != FS_KEY_DERIVATION_NONCE_SIZE);
 	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
 	if (ctx.version != FSCRYPT_CONTEXT_V1)
-		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
+		memcpy(ctx.key_hash, key_hash, FSCRYPT_KEY_HASH_SIZE);
 
 	return inode->i_sb->s_cop->set_context(inode, &ctx,
 					       fscrypt_context_size(&ctx),
@@ -87,6 +91,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 	struct inode *inode = file_inode(filp);
 	int ret;
 	struct fscrypt_context ctx;
+	u8 key_hash[FSCRYPT_KEY_HASH_SIZE];
 
 	if (copy_from_user(&policy, arg, sizeof(policy)))
 		return -EFAULT;
@@ -98,6 +103,25 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 	    policy.version != FS_POLICY_VERSION_HKDF)
 		return -EINVAL;
 
+	if (policy.version == FS_POLICY_VERSION_ORIGINAL) {
+		/*
+		 * Originally no key verification was implemented, which was
+		 * insufficient for scenarios where multiple users share
+		 * encrypted files.  The new encryption policy version fixes
+		 * this and also implements an improved key derivation function.
+		 * So as long as the key can be in the keyring at the time the
+		 * policy is set and compatibility with old kernels isn't
+		 * required, it's recommended to use the new policy version
+		 * (fscrypt_policy.version = 2).
+		 */
+		pr_warn_once("%s (pid %d) is setting less secure v0 encryption policy; recommend upgrading to v2.\n",
+			     current->comm, current->pid);
+	} else {
+		ret = fscrypt_compute_key_hash(inode, &policy, key_hash);
+		if (ret)
+			return ret;
+	}
+
 	ret = mnt_want_write_file(filp);
 	if (ret)
 		return ret;
@@ -112,10 +136,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 			ret = -ENOTEMPTY;
 		else
 			ret = create_encryption_context_from_policy(inode,
-								    &policy);
+								    &policy,
+								    key_hash);
 	} else if (ret >= 0 && fscrypt_valid_context_format(&ctx, ret) &&
 		   is_encryption_context_consistent_with_policy(&ctx,
-								&policy)) {
+								&policy,
+								key_hash)) {
 		/* The file already uses the same encryption policy. */
 		ret = 0;
 	} else if (ret >= 0 || ret == -ERANGE) {
@@ -232,7 +258,11 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
 			(parent_ci->ci_filename_mode ==
 			 child_ci->ci_filename_mode) &&
-			(parent_ci->ci_flags == child_ci->ci_flags);
+			(parent_ci->ci_flags == child_ci->ci_flags) &&
+			(parent_ci->ci_context_version == FSCRYPT_CONTEXT_V1 ||
+			 (memcmp(parent_ci->ci_master_key->mk_hash,
+				 child_ci->ci_master_key->mk_hash,
+				 FSCRYPT_KEY_HASH_SIZE) == 0));
 	}
 
 	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
@@ -251,7 +281,10 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 		 child_ctx.contents_encryption_mode) &&
 		(parent_ctx.filenames_encryption_mode ==
 		 child_ctx.filenames_encryption_mode) &&
-		(parent_ctx.flags == child_ctx.flags);
+		(parent_ctx.flags == child_ctx.flags) &&
+		(parent_ctx.version == FSCRYPT_CONTEXT_V1 ||
+		 (memcmp(parent_ctx.key_hash, child_ctx.key_hash,
+			 FSCRYPT_KEY_HASH_SIZE) == 0));
 }
 EXPORT_SYMBOL(fscrypt_has_permitted_context);
 
@@ -286,8 +319,10 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 	memcpy(ctx.master_key_descriptor, ci->ci_master_key_descriptor,
 	       FS_KEY_DESCRIPTOR_SIZE);
 	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
-	if (ctx.version != FSCRYPT_CONTEXT_V1)
-		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
+	if (ctx.version != FSCRYPT_CONTEXT_V1) {
+		memcpy(ctx.key_hash, ci->ci_master_key->mk_hash,
+		       FSCRYPT_KEY_HASH_SIZE);
+	}
 
 	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
 	res = parent->i_sb->s_cop->set_context(child, &ctx,
-- 
2.13.2.932.g7449e964c-goog

Re: [PATCH 4/6] fscrypt: verify that the correct master key was supplied

[Michael Halcrow via Linux-f2fs-devel]

On Wed, Jul 12, 2017 at 02:00:33PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, while a fscrypt master key is required to have a certain
> description in the keyring, its payload is never verified to be correct.
> While sufficient for well-behaved userspace, this is insecure in a
> multi-user system where a user has been given only read-only access to
> an encrypted file or directory.  Specifically, if an encrypted file or
> directory does not yet have its key cached by the kernel, the first user
> who accesses it can provide an arbitrary key in their own keyring, which
> the kernel will then associate with the inode and use for read(),
> write(), readdir(), etc. by other users as well.
> 
> Consequently, it's trivial for a user with *read-only* access to an
> encrypted file or directory to make it appear as garbage to everyone.
> Creative users might be able to accomplish more sophisticated attacks by
> careful choice of the key, e.g. choosing a key causes certain bytes of
> file contents to have certain values or that causes filenames containing
> the '/' character to appear.
> 
> Solve the problem for v2 encryption policies by storing a "hash" of the
> master encryption key in the encryption xattr and verifying it before
> accepting the user-provided key.  We generate the "hash" using
> HKDF-SHA512 by passing a distinct application-specific info string.
> This produces a value which is cryptographically isolated and can be
> stored in the clear without leaking any information about the master key
> or any other derived keys (in a computational sense).  Reusing HKDF is
> better than doing e.g. SHA-512(master_key) because it avoids passing the
> same key into different cryptographic primitives.
> 
> We make the hash field 16 bytes long, as this should provide sufficient
> collision and preimage resistance while not wasting too much space for
> the encryption xattr.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Michael Halcrow <mhalcrow@google.com>

> ---
>  fs/crypto/fscrypt_private.h |  4 ++++
>  fs/crypto/keyinfo.c         | 46 +++++++++++++++++++++++++++++++++++++
>  fs/crypto/policy.c          | 55 ++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 095e7c16483a..a7baeac92575 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -92,6 +92,7 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
>  struct fscrypt_master_key {
>  	struct crypto_shash	*mk_hmac;
>  	unsigned int		mk_size;
> +	u8			mk_hash[FSCRYPT_KEY_HASH_SIZE];
>  };
>  
>  /*
> @@ -155,6 +156,9 @@ extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
>  					      gfp_t gfp_flags);
>  
>  /* keyinfo.c */
> +extern int fscrypt_compute_key_hash(const struct inode *inode,
> +				    const struct fscrypt_policy *policy,
> +				    u8 hash[FSCRYPT_KEY_HASH_SIZE]);
>  extern void __exit fscrypt_essiv_cleanup(void);
>  
>  #endif /* _FSCRYPT_PRIVATE_H */
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 7ed1a7fb1308..12a60eacf819 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -39,8 +39,11 @@ static struct crypto_shash *essiv_hash_tfm;
>   *
>   * Keys derived with different info strings are cryptographically isolated from
>   * each other --- knowledge of one derived key doesn't reveal any others.
> + * (This property is particularly important for the derived key used as the
> + * "key hash", as that is stored in the clear.)
>   */
>  #define HKDF_CONTEXT_PER_FILE_KEY	1
> +#define HKDF_CONTEXT_KEY_HASH		2
>  
>  /*
>   * HKDF consists of two steps:
> @@ -212,6 +215,12 @@ alloc_master_key(const struct fscrypt_key *payload)
>  	err = crypto_shash_setkey(k->mk_hmac, prk, sizeof(prk));
>  	if (err)
>  		goto fail;
> +
> +	/* Calculate the "key hash" */
> +	err = hkdf_expand(k->mk_hmac, HKDF_CONTEXT_KEY_HASH, NULL, 0,
> +			  k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> +	if (err)
> +		goto fail;
>  out:
>  	memzero_explicit(prk, sizeof(prk));
>  	return k;
> @@ -537,6 +546,31 @@ void __exit fscrypt_essiv_cleanup(void)
>  	crypto_free_shash(essiv_hash_tfm);
>  }
>  
> +int fscrypt_compute_key_hash(const struct inode *inode,
> +			     const struct fscrypt_policy *policy,
> +			     u8 hash[FSCRYPT_KEY_HASH_SIZE])
> +{
> +	struct fscrypt_master_key *k;
> +	unsigned int min_keysize;
> +
> +	/*
> +	 * Require that the master key be long enough for both the
> +	 * contents and filenames encryption modes.
> +	 */
> +	min_keysize =
> +		max(available_modes[policy->contents_encryption_mode].keysize,
> +		    available_modes[policy->filenames_encryption_mode].keysize);
> +
> +	k = load_master_key_from_keyring(inode, policy->master_key_descriptor,
> +					 min_keysize);
> +	if (IS_ERR(k))
> +		return PTR_ERR(k);
> +
> +	memcpy(hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> +	put_master_key(k);
> +	return 0;
> +}
> +
>  int fscrypt_get_encryption_info(struct inode *inode)
>  {
>  	struct fscrypt_info *crypt_info;
> @@ -613,6 +647,18 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  			goto out;
>  		}
>  
> +		/*
> +		 * Make sure the master key we found has the correct hash.
> +		 * Buggy or malicious userspace may provide the wrong key.
> +		 */
> +		if (memcmp(crypt_info->ci_master_key->mk_hash, ctx.key_hash,
> +			   FSCRYPT_KEY_HASH_SIZE)) {
> +			pr_warn_ratelimited("fscrypt: wrong encryption key supplied for inode %lu\n",
> +					    inode->i_ino);
> +			res = -ENOKEY;
> +			goto out;
> +		}
> +
>  		res = derive_key_hkdf(crypt_info->ci_master_key, &ctx,
>  				      derived_key, derived_keysize);
>  	}
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 81c59f8e45c0..2934bc2bff4b 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -40,7 +40,8 @@ static u8 context_version_for_policy(const struct fscrypt_policy *policy)
>   */
>  static bool is_encryption_context_consistent_with_policy(
>  				const struct fscrypt_context *ctx,
> -				const struct fscrypt_policy *policy)
> +				const struct fscrypt_policy *policy,
> +				const u8 key_hash[FSCRYPT_KEY_HASH_SIZE])
>  {
>  	return (ctx->version == context_version_for_policy(policy)) &&
>  		(memcmp(ctx->master_key_descriptor,
> @@ -50,11 +51,14 @@ static bool is_encryption_context_consistent_with_policy(
>  		(ctx->contents_encryption_mode ==
>  		 policy->contents_encryption_mode) &&
>  		(ctx->filenames_encryption_mode ==
> -		 policy->filenames_encryption_mode);
> +		 policy->filenames_encryption_mode) &&
> +		(ctx->version == FSCRYPT_CONTEXT_V1 ||
> +		 (memcmp(ctx->key_hash, key_hash, FSCRYPT_KEY_HASH_SIZE) == 0));
>  }
>  
>  static int create_encryption_context_from_policy(struct inode *inode,
> -				const struct fscrypt_policy *policy)
> +				const struct fscrypt_policy *policy,
> +				const u8 key_hash[FSCRYPT_KEY_HASH_SIZE])
>  {
>  	struct fscrypt_context ctx;
>  
> @@ -74,7 +78,7 @@ static int create_encryption_context_from_policy(struct inode *inode,
>  	BUILD_BUG_ON(sizeof(ctx.nonce) != FS_KEY_DERIVATION_NONCE_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
>  	if (ctx.version != FSCRYPT_CONTEXT_V1)
> -		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
> +		memcpy(ctx.key_hash, key_hash, FSCRYPT_KEY_HASH_SIZE);
>  
>  	return inode->i_sb->s_cop->set_context(inode, &ctx,
>  					       fscrypt_context_size(&ctx),
> @@ -87,6 +91,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  	struct inode *inode = file_inode(filp);
>  	int ret;
>  	struct fscrypt_context ctx;
> +	u8 key_hash[FSCRYPT_KEY_HASH_SIZE];
>  
>  	if (copy_from_user(&policy, arg, sizeof(policy)))
>  		return -EFAULT;
> @@ -98,6 +103,25 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  	    policy.version != FS_POLICY_VERSION_HKDF)
>  		return -EINVAL;
>  
> +	if (policy.version == FS_POLICY_VERSION_ORIGINAL) {
> +		/*
> +		 * Originally no key verification was implemented, which was
> +		 * insufficient for scenarios where multiple users share
> +		 * encrypted files.  The new encryption policy version fixes
> +		 * this and also implements an improved key derivation function.
> +		 * So as long as the key can be in the keyring at the time the
> +		 * policy is set and compatibility with old kernels isn't
> +		 * required, it's recommended to use the new policy version
> +		 * (fscrypt_policy.version = 2).
> +		 */
> +		pr_warn_once("%s (pid %d) is setting less secure v0 encryption policy; recommend upgrading to v2.\n",
> +			     current->comm, current->pid);
> +	} else {
> +		ret = fscrypt_compute_key_hash(inode, &policy, key_hash);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = mnt_want_write_file(filp);
>  	if (ret)
>  		return ret;
> @@ -112,10 +136,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  			ret = -ENOTEMPTY;
>  		else
>  			ret = create_encryption_context_from_policy(inode,
> -								    &policy);
> +								    &policy,
> +								    key_hash);
>  	} else if (ret >= 0 && fscrypt_valid_context_format(&ctx, ret) &&
>  		   is_encryption_context_consistent_with_policy(&ctx,
> -								&policy)) {
> +								&policy,
> +								key_hash)) {
>  		/* The file already uses the same encryption policy. */
>  		ret = 0;
>  	} else if (ret >= 0 || ret == -ERANGE) {
> @@ -232,7 +258,11 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
>  			(parent_ci->ci_filename_mode ==
>  			 child_ci->ci_filename_mode) &&
> -			(parent_ci->ci_flags == child_ci->ci_flags);
> +			(parent_ci->ci_flags == child_ci->ci_flags) &&
> +			(parent_ci->ci_context_version == FSCRYPT_CONTEXT_V1 ||
> +			 (memcmp(parent_ci->ci_master_key->mk_hash,
> +				 child_ci->ci_master_key->mk_hash,
> +				 FSCRYPT_KEY_HASH_SIZE) == 0));
>  	}
>  
>  	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
> @@ -251,7 +281,10 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  		 child_ctx.contents_encryption_mode) &&
>  		(parent_ctx.filenames_encryption_mode ==
>  		 child_ctx.filenames_encryption_mode) &&
> -		(parent_ctx.flags == child_ctx.flags);
> +		(parent_ctx.flags == child_ctx.flags) &&
> +		(parent_ctx.version == FSCRYPT_CONTEXT_V1 ||
> +		 (memcmp(parent_ctx.key_hash, child_ctx.key_hash,
> +			 FSCRYPT_KEY_HASH_SIZE) == 0));
>  }
>  EXPORT_SYMBOL(fscrypt_has_permitted_context);
>  
> @@ -286,8 +319,10 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  	memcpy(ctx.master_key_descriptor, ci->ci_master_key_descriptor,
>  	       FS_KEY_DESCRIPTOR_SIZE);
>  	get_random_bytes(ctx.nonce, FS_KEY_DERIVATION_NONCE_SIZE);
> -	if (ctx.version != FSCRYPT_CONTEXT_V1)
> -		memset(ctx.key_hash, 0, FSCRYPT_KEY_HASH_SIZE);
> +	if (ctx.version != FSCRYPT_CONTEXT_V1) {
> +		memcpy(ctx.key_hash, ci->ci_master_key->mk_hash,
> +		       FSCRYPT_KEY_HASH_SIZE);
> +	}
>  
>  	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
>  	res = parent->i_sb->s_cop->set_context(child, &ctx,
> -- 
> 2.13.2.932.g7449e964c-goog
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 4/6] fscrypt: verify that the correct master key was supplied

[Jeffrey Walton]

On Wed, Jul 12, 2017 at 5:00 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
>....
> Solve the problem for v2 encryption policies by storing a "hash" of the
> master encryption key in the encryption xattr and verifying it before
> accepting the user-provided key.
> ...

Forgive my ignorance... Doesn't that setup an oracle so an attacker
can query keys?

It seems like the problem is deeper into the design. Namely, the
caching and sharing of keys.

Jeff

Re: [PATCH 4/6] fscrypt: verify that the correct master key was supplied

[Eric Biggers]

Hi Jeff,

On Fri, Jul 14, 2017 at 01:34:48PM -0400, Jeffrey Walton wrote:
> On Wed, Jul 12, 2017 at 5:00 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> >....
> > Solve the problem for v2 encryption policies by storing a "hash" of the
> > master encryption key in the encryption xattr and verifying it before
> > accepting the user-provided key.
> > ...
> 
> Forgive my ignorance... Doesn't that setup an oracle so an attacker
> can query keys?
> 
> It seems like the problem is deeper into the design. Namely, the
> caching and sharing of keys.
> 

Your concerns are a little vague, so I'll try to cover both major attack
scenarios.  We can assume that key_hash is public information, available to the
attacker.  (For offline attacks that's obviously the case.  For online attacks
it's not obvious since no API exposes it yet, but we'll consider it available
nonetheless, perhaps via a side-channel attack.)

The first attack scenario is breaking the encryption itself, and specifically
whether having key_hash makes it easier.  Fundamentally, adding a 128-bit
key_hash reduces by a factor of 2^128 the number of keys which may be the
correct one.  Thus, to not weaken the encryption, it needs to be infeasible to
enumerate the possible keys which hash to a particular value.  If that could be
done faster than brute force, then recovering the key would in turn be faster
than brute force, so the cryptosystem would be broken in a theoretical sense.

However, in practice I don't think we should be too concerned about it.  SHA-512
has been holding up pretty well to preimage attacks, and enumerating all
possible source messages of a particular length is much more specific than even
a preimage attack actually.  And even if this scheme were, incredibly, fully
broken to the extent that the hash might as well just be the first 128 bits of
the key, if the recommended AES-256-XTS mode is used the master key is actually
512 bits, so there would *still* be 384 bits remaining to brute-force.

The second attack scenario would be not breaking the encryption directly, but
rather causing "someone else's" data to be encrypted (or decrypted) with the
wrong key, or even left unencrypted.  I'll limit the focus to online attacks,
since offline attacks are a different story for this.  Previously we had no
protection against this attack beyond visibility of files: anyone with *read*
access to someone's encrypted directory or files could provision the wrong key.
With this patchset this problem is solved, at least in practice: to provision
the wrong key, the attacker will now need to compute a preimage of key_hash,
which as noted is intended to be computationally expensive.  Note that
brute-forcing a 128-bit hash's preimage would take over 10 million years if you
had 1 trillion computers each performing 1 trillion hashes per second --- so
clearly a practical attack would have to be much more clever.

Now, even if a preimage could be found much more easily, as long as it still had
a decent cost I really, really doubt that an attacker --- who, given that the
attack scenario is *online* very likely already has code execution on the system
--- would perform an expensive cryptographic attack to *maybe* gain the ability
to later decrypt some particular new file contents belonging to one particular
user on one particular device, vs. just exploiting a security vulnerability to
elevate privileges or gain kernel code execution, then reading everything from
memory.  Remember, attacks take the path of least resistance.

Therefore, I actually felt confident enough in the hash to use it as the only
identifier in ->s_master_keys, even though that expands the "providing the wrong
key" attack surface to the whole filesystem rather than just the files visible
to the attacker.  If we are truly concerned about this, we could add raw_key to
struct fscrypt_master_key, always do the keyring lookup, and compare the raw_key
in the keyring key with the fscrypt_master_key.  It would arguably be a step
backwards towards relying less on keyrings and their visibility problems, and it
would leave another copy of the master key in memory, but it could be done.

I think another scenario which you may be getting at is whether the presence of
key_hash makes it easier to test whether a candidate key is the right one.  The
answer is potentially yes, but it doesn't really matter.  If an attacker is
actually able to enumerate your key, then you are already screwed as they can
likely validate it in some other efficient way, e.g. by encrypting some known
plaintext blocks and comparing it to the ciphertext.  If your "encryption" key
doesn't contain enough randomness to prevent it from being enumerated in a
practical sense then it's not encryption --- it's obfuscation.

Eric

[PATCH 5/6] fscrypt: cache the HMAC transform for each master key

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Now that we have a key_hash field which securely identifies a master key
payload, introduce a cache of the HMAC transforms for the master keys
currently in use for inodes using v2+ encryption policies.  The entries
in this cache are called 'struct fscrypt_master_key' and are identified
by key_hash.  The cache is per-superblock.  (It could be global, but
making it per-superblock should reduce the lock contention a bit, and we
may need to keep track of keys on a per-superblock basis for other
reasons later, e.g. to implement an ioctl for evicting keys.)

This results in a large efficiency gain: we now only have to allocate
and key an "hmac(sha512)" transformation, execute HKDF-Extract, and
compute key_hash once per master key rather than once per inode.  Note
that this optimization can't easily be applied to the original AES-based
KDF because that uses a different AES key for each KDF execution.  In
practice, this difference makes the HKDF per-inode encryption key
derivation performance comparable to or even faster than the old KDF,
which typically spends more time allocating an "ecb(aes)" transformation
from the crypto API than doing actual crypto work.

Note that it would have been possible to make the mapping be from
raw_key => fscrypt_master_key (where raw_key denotes the actual bytes of
the master key) rather than from key_hash => fscrypt_master_key.
However, an advantage of doing lookups by key_hash is that it replaces
the keyring lookup in most cases, which opens up the future
possibilities of not even having the master key in memory following an
initial provisioning step (if the HMAC-SHA512 implementation is
hardware-backed), or of introducing an ioctl to provision a key to the
filesystem directly, avoiding keyrings and their visibility problems
entirely.  Also, because key_hash is public information while raw_key is
secret information, it would have been very difficult to use raw_key as
a map key in a way that would prevent timing attacks while still being
scalable to a large number of entries.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h |  11 ++++
 fs/crypto/keyinfo.c         | 134 +++++++++++++++++++++++++++++++++++++++++++-
 fs/crypto/policy.c          |   5 +-
 fs/super.c                  |   4 ++
 include/linux/fs.h          |   5 ++
 5 files changed, 152 insertions(+), 7 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index a7baeac92575..4b158717a8c3 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -88,11 +88,22 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
 
 /*
  * fscrypt_master_key - an in-use master key
+ *
+ * This is referenced from each in-core inode that has been "unlocked" using a
+ * particular master key.  It's primarily used to cache the HMAC transform so
+ * that the per-inode encryption keys can be derived efficiently with HKDF.  It
+ * is securely erased once all inodes referencing it have been evicted.
+ *
+ * If the same master key is used on different filesystems (unusual, but
+ * possible), we'll create one of these structs for each filesystem.
  */
 struct fscrypt_master_key {
 	struct crypto_shash	*mk_hmac;
 	unsigned int		mk_size;
 	u8			mk_hash[FSCRYPT_KEY_HASH_SIZE];
+	refcount_t		mk_refcount;
+	struct rb_node		mk_node;
+	struct super_block	*mk_sb;
 };
 
 /*
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 12a60eacf819..bf60e76f9599 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -176,6 +176,14 @@ static void put_master_key(struct fscrypt_master_key *k)
 	if (!k)
 		return;
 
+	if (refcount_read(&k->mk_refcount) != 0) { /* in ->s_master_keys? */
+		if (!refcount_dec_and_lock(&k->mk_refcount,
+					   &k->mk_sb->s_master_keys_lock))
+			return;
+		rb_erase(&k->mk_node, &k->mk_sb->s_master_keys);
+		spin_unlock(&k->mk_sb->s_master_keys_lock);
+	}
+
 	crypto_free_shash(k->mk_hmac);
 	kzfree(k);
 }
@@ -231,6 +239,87 @@ alloc_master_key(const struct fscrypt_key *payload)
 	goto out;
 }
 
+/*
+ * ->s_master_keys is a map of master keys currently in use by in-core inodes on
+ * a given filesystem, identified by key_hash which is a cryptographically
+ * secure identifier for an actual key payload.
+ *
+ * Note that master_key_descriptor cannot be used to identify the keys because
+ * master_key_descriptor only identifies the "location" of a key in the keyring,
+ * not the actual key payload --- i.e., buggy or malicious userspace may provide
+ * different keys with the same master_key_descriptor.
+ */
+
+/*
+ * Search ->s_master_keys for the fscrypt_master_key having the specified hash.
+ * If found return it with a reference taken, otherwise return NULL.
+ */
+static struct fscrypt_master_key *
+get_cached_master_key(struct super_block *sb,
+		      const u8 hash[FSCRYPT_KEY_HASH_SIZE])
+{
+	struct rb_node *node;
+	struct fscrypt_master_key *k;
+	int res;
+
+	spin_lock(&sb->s_master_keys_lock);
+	node = sb->s_master_keys.rb_node;
+	while (node) {
+		k = rb_entry(node, struct fscrypt_master_key, mk_node);
+		res = memcmp(hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
+		if (res < 0)
+			node = node->rb_left;
+		else if (res > 0)
+			node = node->rb_right;
+		else {
+			refcount_inc(&k->mk_refcount);
+			goto out;
+		}
+	}
+	k = NULL;
+out:
+	spin_unlock(&sb->s_master_keys_lock);
+	return k;
+}
+
+/*
+ * Try to insert the specified fscrypt_master_key into ->s_master_keys.  If it
+ * already exists, then drop the key being inserted and take a reference to the
+ * existing one instead.
+ */
+static struct fscrypt_master_key *
+insert_master_key(struct super_block *sb, struct fscrypt_master_key *new)
+{
+	struct fscrypt_master_key *k;
+	struct rb_node *parent = NULL, **p;
+	int res;
+
+	spin_lock(&sb->s_master_keys_lock);
+	p = &sb->s_master_keys.rb_node;
+	while (*p) {
+		parent = *p;
+		k = rb_entry(parent, struct fscrypt_master_key, mk_node);
+		res = memcmp(new->mk_hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
+		if (res < 0)
+			p = &parent->rb_left;
+		else if (res > 0)
+			p = &parent->rb_right;
+		else {
+			refcount_inc(&k->mk_refcount);
+			spin_unlock(&sb->s_master_keys_lock);
+			put_master_key(new);
+			return k;
+		}
+	}
+
+	rb_link_node(&new->mk_node, parent, p);
+	rb_insert_color(&new->mk_node, &sb->s_master_keys);
+	refcount_set(&new->mk_refcount, 1);
+	new->mk_sb = sb;
+	spin_unlock(&sb->s_master_keys_lock);
+	return new;
+}
+
 static void release_keyring_key(struct key *keyring_key)
 {
 	up_read(&keyring_key->sem);
@@ -321,6 +410,47 @@ load_master_key_from_keyring(const struct inode *inode,
 	return master_key;
 }
 
+/*
+ * Get the fscrypt_master_key identified by the specified v2+ encryption
+ * context, or create it if not found.
+ *
+ * Returns the fscrypt_master_key with a reference taken, or an ERR_PTR().
+ */
+static struct fscrypt_master_key *
+find_or_create_master_key(const struct inode *inode,
+			  const struct fscrypt_context *ctx,
+			  unsigned int min_keysize)
+{
+	struct fscrypt_master_key *master_key;
+
+	if (WARN_ON(ctx->version < FSCRYPT_CONTEXT_V2))
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * First try looking up the master key by its cryptographically secure
+	 * key_hash.  If it's already in memory, there's no need to do a keyring
+	 * search.  (Note that we don't enforce access control based on which
+	 * processes "have the key" and which don't, as encryption is meant to
+	 * be orthogonal to operating-system level access control.  Hence, it's
+	 * sufficient for anyone on the system to have added the needed key.)
+	 */
+	master_key = get_cached_master_key(inode->i_sb, ctx->key_hash);
+	if (master_key)
+		return master_key;
+
+	/*
+	 * The needed master key isn't in memory yet.  Load it from the keyring.
+	 */
+	master_key = load_master_key_from_keyring(inode,
+						  ctx->master_key_descriptor,
+						  min_keysize);
+	if (IS_ERR(master_key))
+		return master_key;
+
+	/* Cache the key for later */
+	return insert_master_key(inode->i_sb, master_key);
+}
+
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
 {
 	struct fscrypt_completion_result *ecr = req->data;
@@ -638,9 +768,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
 					     derived_keysize);
 	} else {
 		crypt_info->ci_master_key =
-			load_master_key_from_keyring(inode,
-						     ctx.master_key_descriptor,
-						     derived_keysize);
+			find_or_create_master_key(inode, &ctx, derived_keysize);
 		if (IS_ERR(crypt_info->ci_master_key)) {
 			res = PTR_ERR(crypt_info->ci_master_key);
 			crypt_info->ci_master_key = NULL;
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 2934bc2bff4b..7661c66a3533 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -259,10 +259,7 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 			(parent_ci->ci_filename_mode ==
 			 child_ci->ci_filename_mode) &&
 			(parent_ci->ci_flags == child_ci->ci_flags) &&
-			(parent_ci->ci_context_version == FSCRYPT_CONTEXT_V1 ||
-			 (memcmp(parent_ci->ci_master_key->mk_hash,
-				 child_ci->ci_master_key->mk_hash,
-				 FSCRYPT_KEY_HASH_SIZE) == 0));
+			(parent_ci->ci_master_key == child_ci->ci_master_key);
 	}
 
 	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
diff --git a/fs/super.c b/fs/super.c
index adb0c0de428c..90bd61ea139c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -215,6 +215,10 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	spin_lock_init(&s->s_inode_list_lock);
 	INIT_LIST_HEAD(&s->s_inodes_wb);
 	spin_lock_init(&s->s_inode_wblist_lock);
+#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
+	s->s_master_keys = RB_ROOT;
+	spin_lock_init(&s->s_master_keys_lock);
+#endif
 
 	if (list_lru_init_memcg(&s->s_dentry_lru))
 		goto fail;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 976aaa1af82a..4f47e1bc81bc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1417,6 +1417,11 @@ struct super_block {
 
 	spinlock_t		s_inode_wblist_lock;
 	struct list_head	s_inodes_wb;	/* writeback inodes */
+
+#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
+	spinlock_t		s_master_keys_lock;
+	struct rb_root		s_master_keys;	/* master crypto keys in use */
+#endif
 };
 
 /* Helper functions so that in most cases filesystems will
-- 
2.13.2.932.g7449e964c-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 5/6] fscrypt: cache the HMAC transform for each master key

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:34PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Now that we have a key_hash field which securely identifies a master key
> payload, introduce a cache of the HMAC transforms for the master keys
> currently in use for inodes using v2+ encryption policies.  The entries
> in this cache are called 'struct fscrypt_master_key' and are identified
> by key_hash.  The cache is per-superblock.  (It could be global, but
> making it per-superblock should reduce the lock contention a bit, and we
> may need to keep track of keys on a per-superblock basis for other
> reasons later, e.g. to implement an ioctl for evicting keys.)
> 
> This results in a large efficiency gain: we now only have to allocate
> and key an "hmac(sha512)" transformation, execute HKDF-Extract, and
> compute key_hash once per master key rather than once per inode.  Note
> that this optimization can't easily be applied to the original AES-based
> KDF because that uses a different AES key for each KDF execution.  In
> practice, this difference makes the HKDF per-inode encryption key
> derivation performance comparable to or even faster than the old KDF,
> which typically spends more time allocating an "ecb(aes)" transformation
> from the crypto API than doing actual crypto work.
> 
> Note that it would have been possible to make the mapping be from
> raw_key => fscrypt_master_key (where raw_key denotes the actual bytes of
> the master key) rather than from key_hash => fscrypt_master_key.
> However, an advantage of doing lookups by key_hash is that it replaces
> the keyring lookup in most cases, which opens up the future
> possibilities of not even having the master key in memory following an
> initial provisioning step (if the HMAC-SHA512 implementation is
> hardware-backed), or of introducing an ioctl to provision a key to the
> filesystem directly, avoiding keyrings and their visibility problems
> entirely.  Also, because key_hash is public information while raw_key is
> secret information, it would have been very difficult to use raw_key as
> a map key in a way that would prevent timing attacks while still being
> scalable to a large number of entries.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Michael Halcrow <mhalcrow@google.com>

> ---
>  fs/crypto/fscrypt_private.h |  11 ++++
>  fs/crypto/keyinfo.c         | 134 +++++++++++++++++++++++++++++++++++++++++++-
>  fs/crypto/policy.c          |   5 +-
>  fs/super.c                  |   4 ++
>  include/linux/fs.h          |   5 ++
>  5 files changed, 152 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index a7baeac92575..4b158717a8c3 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -88,11 +88,22 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
>  
>  /*
>   * fscrypt_master_key - an in-use master key
> + *
> + * This is referenced from each in-core inode that has been "unlocked" using a
> + * particular master key.  It's primarily used to cache the HMAC transform so
> + * that the per-inode encryption keys can be derived efficiently with HKDF.  It
> + * is securely erased once all inodes referencing it have been evicted.
> + *
> + * If the same master key is used on different filesystems (unusual, but
> + * possible), we'll create one of these structs for each filesystem.
>   */
>  struct fscrypt_master_key {
>  	struct crypto_shash	*mk_hmac;
>  	unsigned int		mk_size;
>  	u8			mk_hash[FSCRYPT_KEY_HASH_SIZE];
> +	refcount_t		mk_refcount;
> +	struct rb_node		mk_node;
> +	struct super_block	*mk_sb;
>  };
>  
>  /*
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 12a60eacf819..bf60e76f9599 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -176,6 +176,14 @@ static void put_master_key(struct fscrypt_master_key *k)
>  	if (!k)
>  		return;
>  
> +	if (refcount_read(&k->mk_refcount) != 0) { /* in ->s_master_keys? */
> +		if (!refcount_dec_and_lock(&k->mk_refcount,
> +					   &k->mk_sb->s_master_keys_lock))
> +			return;
> +		rb_erase(&k->mk_node, &k->mk_sb->s_master_keys);
> +		spin_unlock(&k->mk_sb->s_master_keys_lock);
> +	}
> +
>  	crypto_free_shash(k->mk_hmac);
>  	kzfree(k);
>  }
> @@ -231,6 +239,87 @@ alloc_master_key(const struct fscrypt_key *payload)
>  	goto out;
>  }
>  
> +/*
> + * ->s_master_keys is a map of master keys currently in use by in-core inodes on
> + * a given filesystem, identified by key_hash which is a cryptographically
> + * secure identifier for an actual key payload.
> + *
> + * Note that master_key_descriptor cannot be used to identify the keys because
> + * master_key_descriptor only identifies the "location" of a key in the keyring,
> + * not the actual key payload --- i.e., buggy or malicious userspace may provide
> + * different keys with the same master_key_descriptor.
> + */
> +
> +/*
> + * Search ->s_master_keys for the fscrypt_master_key having the specified hash.
> + * If found return it with a reference taken, otherwise return NULL.
> + */
> +static struct fscrypt_master_key *
> +get_cached_master_key(struct super_block *sb,
> +		      const u8 hash[FSCRYPT_KEY_HASH_SIZE])
> +{
> +	struct rb_node *node;
> +	struct fscrypt_master_key *k;
> +	int res;
> +
> +	spin_lock(&sb->s_master_keys_lock);
> +	node = sb->s_master_keys.rb_node;
> +	while (node) {
> +		k = rb_entry(node, struct fscrypt_master_key, mk_node);
> +		res = memcmp(hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> +		if (res < 0)
> +			node = node->rb_left;
> +		else if (res > 0)
> +			node = node->rb_right;
> +		else {
> +			refcount_inc(&k->mk_refcount);
> +			goto out;
> +		}
> +	}
> +	k = NULL;
> +out:
> +	spin_unlock(&sb->s_master_keys_lock);
> +	return k;
> +}
> +
> +/*
> + * Try to insert the specified fscrypt_master_key into ->s_master_keys.  If it
> + * already exists, then drop the key being inserted and take a reference to the
> + * existing one instead.
> + */
> +static struct fscrypt_master_key *
> +insert_master_key(struct super_block *sb, struct fscrypt_master_key *new)
> +{
> +	struct fscrypt_master_key *k;
> +	struct rb_node *parent = NULL, **p;
> +	int res;
> +
> +	spin_lock(&sb->s_master_keys_lock);
> +	p = &sb->s_master_keys.rb_node;
> +	while (*p) {
> +		parent = *p;
> +		k = rb_entry(parent, struct fscrypt_master_key, mk_node);
> +		res = memcmp(new->mk_hash, k->mk_hash, FSCRYPT_KEY_HASH_SIZE);
> +		if (res < 0)
> +			p = &parent->rb_left;
> +		else if (res > 0)
> +			p = &parent->rb_right;
> +		else {
> +			refcount_inc(&k->mk_refcount);
> +			spin_unlock(&sb->s_master_keys_lock);
> +			put_master_key(new);
> +			return k;
> +		}
> +	}
> +
> +	rb_link_node(&new->mk_node, parent, p);
> +	rb_insert_color(&new->mk_node, &sb->s_master_keys);
> +	refcount_set(&new->mk_refcount, 1);
> +	new->mk_sb = sb;
> +	spin_unlock(&sb->s_master_keys_lock);
> +	return new;
> +}
> +
>  static void release_keyring_key(struct key *keyring_key)
>  {
>  	up_read(&keyring_key->sem);
> @@ -321,6 +410,47 @@ load_master_key_from_keyring(const struct inode *inode,
>  	return master_key;
>  }
>  
> +/*
> + * Get the fscrypt_master_key identified by the specified v2+ encryption
> + * context, or create it if not found.
> + *
> + * Returns the fscrypt_master_key with a reference taken, or an ERR_PTR().
> + */
> +static struct fscrypt_master_key *
> +find_or_create_master_key(const struct inode *inode,
> +			  const struct fscrypt_context *ctx,
> +			  unsigned int min_keysize)
> +{
> +	struct fscrypt_master_key *master_key;
> +
> +	if (WARN_ON(ctx->version < FSCRYPT_CONTEXT_V2))
> +		return ERR_PTR(-EINVAL);

Isn't this a programming error?  If so, consider either BUG_ON() or
omit the check.

> +
> +	/*
> +	 * First try looking up the master key by its cryptographically secure
> +	 * key_hash.  If it's already in memory, there's no need to do a keyring
> +	 * search.  (Note that we don't enforce access control based on which
> +	 * processes "have the key" and which don't, as encryption is meant to
> +	 * be orthogonal to operating-system level access control.  Hence, it's
> +	 * sufficient for anyone on the system to have added the needed key.)
> +	 */
> +	master_key = get_cached_master_key(inode->i_sb, ctx->key_hash);
> +	if (master_key)
> +		return master_key;
> +
> +	/*
> +	 * The needed master key isn't in memory yet.  Load it from the keyring.
> +	 */
> +	master_key = load_master_key_from_keyring(inode,
> +						  ctx->master_key_descriptor,
> +						  min_keysize);
> +	if (IS_ERR(master_key))
> +		return master_key;
> +
> +	/* Cache the key for later */
> +	return insert_master_key(inode->i_sb, master_key);
> +}
> +
>  static void derive_crypt_complete(struct crypto_async_request *req, int rc)
>  {
>  	struct fscrypt_completion_result *ecr = req->data;
> @@ -638,9 +768,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  					     derived_keysize);
>  	} else {
>  		crypt_info->ci_master_key =
> -			load_master_key_from_keyring(inode,
> -						     ctx.master_key_descriptor,
> -						     derived_keysize);
> +			find_or_create_master_key(inode, &ctx, derived_keysize);
>  		if (IS_ERR(crypt_info->ci_master_key)) {
>  			res = PTR_ERR(crypt_info->ci_master_key);
>  			crypt_info->ci_master_key = NULL;
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 2934bc2bff4b..7661c66a3533 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -259,10 +259,7 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
>  			(parent_ci->ci_filename_mode ==
>  			 child_ci->ci_filename_mode) &&
>  			(parent_ci->ci_flags == child_ci->ci_flags) &&
> -			(parent_ci->ci_context_version == FSCRYPT_CONTEXT_V1 ||
> -			 (memcmp(parent_ci->ci_master_key->mk_hash,
> -				 child_ci->ci_master_key->mk_hash,
> -				 FSCRYPT_KEY_HASH_SIZE) == 0));
> +			(parent_ci->ci_master_key == child_ci->ci_master_key);
>  	}
>  
>  	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
> diff --git a/fs/super.c b/fs/super.c
> index adb0c0de428c..90bd61ea139c 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -215,6 +215,10 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>  	spin_lock_init(&s->s_inode_list_lock);
>  	INIT_LIST_HEAD(&s->s_inodes_wb);
>  	spin_lock_init(&s->s_inode_wblist_lock);
> +#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
> +	s->s_master_keys = RB_ROOT;
> +	spin_lock_init(&s->s_master_keys_lock);
> +#endif
>  
>  	if (list_lru_init_memcg(&s->s_dentry_lru))
>  		goto fail;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 976aaa1af82a..4f47e1bc81bc 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1417,6 +1417,11 @@ struct super_block {
>  
>  	spinlock_t		s_inode_wblist_lock;
>  	struct list_head	s_inodes_wb;	/* writeback inodes */
> +
> +#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
> +	spinlock_t		s_master_keys_lock;
> +	struct rb_root		s_master_keys;	/* master crypto keys in use */
> +#endif
>  };
>  
>  /* Helper functions so that in most cases filesystems will
> -- 
> 2.13.2.932.g7449e964c-goog
> 

Re: [PATCH 5/6] fscrypt: cache the HMAC transform for each master key

[Eric Biggers]

On Mon, Jul 17, 2017 at 10:45:51AM -0700, Michael Halcrow wrote:
> > +/*
> > + * Get the fscrypt_master_key identified by the specified v2+ encryption
> > + * context, or create it if not found.
> > + *
> > + * Returns the fscrypt_master_key with a reference taken, or an ERR_PTR().
> > + */
> > +static struct fscrypt_master_key *
> > +find_or_create_master_key(const struct inode *inode,
> > +			  const struct fscrypt_context *ctx,
> > +			  unsigned int min_keysize)
> > +{
> > +	struct fscrypt_master_key *master_key;
> > +
> > +	if (WARN_ON(ctx->version < FSCRYPT_CONTEXT_V2))
> > +		return ERR_PTR(-EINVAL);
> 
> Isn't this a programming error?  If so, consider either BUG_ON() or
> omit the check.
> 

Yes, but BUG_ON() is discouraged in cases where there is a straightforward way
to recover from the error.  checkpatch actually warns about using it these days.

Eric

[PATCH 6/6] fscrypt: for v2 policies, support "fscrypt:" key prefix only

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since v2 encryption policies are opt-in, take the opportunity to also
drop support for the legacy filesystem-specific key description prefixes
"ext4:", "f2fs:", and "ubifs:", instead requiring the generic prefix
"fscrypt:".  The generic prefix is preferred since it works for all
filesystems.  Also there is a performance benefit from not having to
search the keyrings twice.

The old prefixes remain supported for v1 encryption policies.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h |  3 +--
 fs/crypto/keyinfo.c         | 16 ++++------------
 fs/crypto/policy.c          |  2 +-
 3 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 4b158717a8c3..201906ff7033 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -167,8 +167,7 @@ extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
 
 /* keyinfo.c */
-extern int fscrypt_compute_key_hash(const struct inode *inode,
-				    const struct fscrypt_policy *policy,
+extern int fscrypt_compute_key_hash(const struct fscrypt_policy *policy,
 				    u8 hash[FSCRYPT_KEY_HASH_SIZE]);
 extern void __exit fscrypt_essiv_cleanup(void);
 
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index bf60e76f9599..e20b5e85c1b3 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -385,8 +385,7 @@ find_and_lock_keyring_key(const char *prefix,
 }
 
 static struct fscrypt_master_key *
-load_master_key_from_keyring(const struct inode *inode,
-			     const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
+load_master_key_from_keyring(const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
 			     unsigned int min_keysize)
 {
 	struct key *keyring_key;
@@ -395,11 +394,6 @@ load_master_key_from_keyring(const struct inode *inode,
 
 	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX, descriptor,
 						min_keysize, &payload);
-	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
-		keyring_key = find_and_lock_keyring_key(
-					inode->i_sb->s_cop->key_prefix,
-					descriptor, min_keysize, &payload);
-	}
 	if (IS_ERR(keyring_key))
 		return ERR_CAST(keyring_key);
 
@@ -441,8 +435,7 @@ find_or_create_master_key(const struct inode *inode,
 	/*
 	 * The needed master key isn't in memory yet.  Load it from the keyring.
 	 */
-	master_key = load_master_key_from_keyring(inode,
-						  ctx->master_key_descriptor,
+	master_key = load_master_key_from_keyring(ctx->master_key_descriptor,
 						  min_keysize);
 	if (IS_ERR(master_key))
 		return master_key;
@@ -676,8 +669,7 @@ void __exit fscrypt_essiv_cleanup(void)
 	crypto_free_shash(essiv_hash_tfm);
 }
 
-int fscrypt_compute_key_hash(const struct inode *inode,
-			     const struct fscrypt_policy *policy,
+int fscrypt_compute_key_hash(const struct fscrypt_policy *policy,
 			     u8 hash[FSCRYPT_KEY_HASH_SIZE])
 {
 	struct fscrypt_master_key *k;
@@ -691,7 +683,7 @@ int fscrypt_compute_key_hash(const struct inode *inode,
 		max(available_modes[policy->contents_encryption_mode].keysize,
 		    available_modes[policy->filenames_encryption_mode].keysize);
 
-	k = load_master_key_from_keyring(inode, policy->master_key_descriptor,
+	k = load_master_key_from_keyring(policy->master_key_descriptor,
 					 min_keysize);
 	if (IS_ERR(k))
 		return PTR_ERR(k);
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 7661c66a3533..cd8c9c7cc9a9 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -117,7 +117,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 		pr_warn_once("%s (pid %d) is setting less secure v0 encryption policy; recommend upgrading to v2.\n",
 			     current->comm, current->pid);
 	} else {
-		ret = fscrypt_compute_key_hash(inode, &policy, key_hash);
+		ret = fscrypt_compute_key_hash(&policy, key_hash);
 		if (ret)
 			return ret;
 	}
-- 
2.13.2.932.g7449e964c-goog

Re: [PATCH 6/6] fscrypt: for v2 policies, support "fscrypt:" key prefix only

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:35PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since v2 encryption policies are opt-in, take the opportunity to also
> drop support for the legacy filesystem-specific key description prefixes
> "ext4:", "f2fs:", and "ubifs:", instead requiring the generic prefix
> "fscrypt:".  The generic prefix is preferred since it works for all
> filesystems.  Also there is a performance benefit from not having to
> search the keyrings twice.
> 
> The old prefixes remain supported for v1 encryption policies.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Michael Halcrow <mhalcrow@google.com>

> ---
>  fs/crypto/fscrypt_private.h |  3 +--
>  fs/crypto/keyinfo.c         | 16 ++++------------
>  fs/crypto/policy.c          |  2 +-
>  3 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 4b158717a8c3..201906ff7033 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -167,8 +167,7 @@ extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
>  					      gfp_t gfp_flags);
>  
>  /* keyinfo.c */
> -extern int fscrypt_compute_key_hash(const struct inode *inode,
> -				    const struct fscrypt_policy *policy,
> +extern int fscrypt_compute_key_hash(const struct fscrypt_policy *policy,
>  				    u8 hash[FSCRYPT_KEY_HASH_SIZE]);
>  extern void __exit fscrypt_essiv_cleanup(void);
>  
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index bf60e76f9599..e20b5e85c1b3 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -385,8 +385,7 @@ find_and_lock_keyring_key(const char *prefix,
>  }
>  
>  static struct fscrypt_master_key *
> -load_master_key_from_keyring(const struct inode *inode,
> -			     const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
> +load_master_key_from_keyring(const u8 descriptor[FS_KEY_DESCRIPTOR_SIZE],
>  			     unsigned int min_keysize)
>  {
>  	struct key *keyring_key;
> @@ -395,11 +394,6 @@ load_master_key_from_keyring(const struct inode *inode,
>  
>  	keyring_key = find_and_lock_keyring_key(FS_KEY_DESC_PREFIX, descriptor,
>  						min_keysize, &payload);
> -	if (keyring_key == ERR_PTR(-ENOKEY) && inode->i_sb->s_cop->key_prefix) {
> -		keyring_key = find_and_lock_keyring_key(
> -					inode->i_sb->s_cop->key_prefix,
> -					descriptor, min_keysize, &payload);
> -	}
>  	if (IS_ERR(keyring_key))
>  		return ERR_CAST(keyring_key);
>  
> @@ -441,8 +435,7 @@ find_or_create_master_key(const struct inode *inode,
>  	/*
>  	 * The needed master key isn't in memory yet.  Load it from the keyring.
>  	 */
> -	master_key = load_master_key_from_keyring(inode,
> -						  ctx->master_key_descriptor,
> +	master_key = load_master_key_from_keyring(ctx->master_key_descriptor,
>  						  min_keysize);
>  	if (IS_ERR(master_key))
>  		return master_key;
> @@ -676,8 +669,7 @@ void __exit fscrypt_essiv_cleanup(void)
>  	crypto_free_shash(essiv_hash_tfm);
>  }
>  
> -int fscrypt_compute_key_hash(const struct inode *inode,
> -			     const struct fscrypt_policy *policy,
> +int fscrypt_compute_key_hash(const struct fscrypt_policy *policy,
>  			     u8 hash[FSCRYPT_KEY_HASH_SIZE])
>  {
>  	struct fscrypt_master_key *k;
> @@ -691,7 +683,7 @@ int fscrypt_compute_key_hash(const struct inode *inode,
>  		max(available_modes[policy->contents_encryption_mode].keysize,
>  		    available_modes[policy->filenames_encryption_mode].keysize);
>  
> -	k = load_master_key_from_keyring(inode, policy->master_key_descriptor,
> +	k = load_master_key_from_keyring(policy->master_key_descriptor,
>  					 min_keysize);
>  	if (IS_ERR(k))
>  		return PTR_ERR(k);
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 7661c66a3533..cd8c9c7cc9a9 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -117,7 +117,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
>  		pr_warn_once("%s (pid %d) is setting less secure v0 encryption policy; recommend upgrading to v2.\n",
>  			     current->comm, current->pid);
>  	} else {
> -		ret = fscrypt_compute_key_hash(inode, &policy, key_hash);
> +		ret = fscrypt_compute_key_hash(&policy, key_hash);
>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.13.2.932.g7449e964c-goog
>