fscrypt: invoke crypto API for ESSIV handling
diff mbox series

Message ID 20191009233840.224128-1-ebiggers@kernel.org
State New
Headers show
Series
  • fscrypt: invoke crypto API for ESSIV handling
Related show

Commit Message

Eric Biggers Oct. 9, 2019, 11:38 p.m. UTC
From: Eric Biggers <ebiggers@google.com>

Instead of open-coding the calculations for ESSIV handling, use an ESSIV
skcipher which does all of this under the hood.  ESSIV was added to the
crypto API in v5.4.

This is based on a patch from Ard Biesheuvel, but reworked to apply
after all the fscrypt changes that went into v5.4.

Tested with 'kvm-xfstests -c ext4,f2fs -g encrypt', including the
ciphertext verification tests for v1 and v2 encryption policies.

Originally-from: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fscrypt.rst |   5 +-
 fs/crypto/crypto.c                    |   4 -
 fs/crypto/fscrypt_private.h           |   7 --
 fs/crypto/keysetup.c                  | 110 +++-----------------------
 fs/crypto/keysetup_v1.c               |   4 -
 5 files changed, 14 insertions(+), 116 deletions(-)

Comments

Ard Biesheuvel Oct. 10, 2019, 5:52 a.m. UTC | #1
On Thu, 10 Oct 2019 at 01:39, Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> Instead of open-coding the calculations for ESSIV handling, use an ESSIV
> skcipher which does all of this under the hood.  ESSIV was added to the
> crypto API in v5.4.
>
> This is based on a patch from Ard Biesheuvel, but reworked to apply
> after all the fscrypt changes that went into v5.4.
>
> Tested with 'kvm-xfstests -c ext4,f2fs -g encrypt', including the
> ciphertext verification tests for v1 and v2 encryption policies.
>
> Originally-from: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks for picking this up.

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

[in case it matters :-)]


> ---
>  Documentation/filesystems/fscrypt.rst |   5 +-
>  fs/crypto/crypto.c                    |   4 -
>  fs/crypto/fscrypt_private.h           |   7 --
>  fs/crypto/keysetup.c                  | 110 +++-----------------------
>  fs/crypto/keysetup_v1.c               |   4 -
>  5 files changed, 14 insertions(+), 116 deletions(-)
>
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 8a0700af95967..6ec459be3de16 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -308,8 +308,9 @@ If unsure, you should use the (AES-256-XTS, AES-256-CTS-CBC) pair.
>
>  AES-128-CBC was added only for low-powered embedded devices with
>  crypto accelerators such as CAAM or CESA that do not support XTS.  To
> -use AES-128-CBC, CONFIG_CRYPTO_SHA256 (or another SHA-256
> -implementation) must be enabled so that ESSIV can be used.
> +use AES-128-CBC, CONFIG_CRYPTO_ESSIV and CONFIG_CRYPTO_SHA256 (or
> +another SHA-256 implementation) must be enabled so that ESSIV can be
> +used.
>
>  Adiantum is a (primarily) stream cipher-based mode that is fast even
>  on CPUs without dedicated crypto instructions.  It's also a true
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 32a7ad0098cc2..6bc3e4f1e657e 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -27,7 +27,6 @@
>  #include <linux/ratelimit.h>
>  #include <linux/dcache.h>
>  #include <linux/namei.h>
> -#include <crypto/aes.h>
>  #include <crypto/skcipher.h>
>  #include "fscrypt_private.h"
>
> @@ -143,9 +142,6 @@ void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
>
>         if (fscrypt_is_direct_key_policy(&ci->ci_policy))
>                 memcpy(iv->nonce, ci->ci_nonce, FS_KEY_DERIVATION_NONCE_SIZE);
> -
> -       if (ci->ci_essiv_tfm != NULL)
> -               crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv->raw, iv->raw);
>  }
>
>  /* Encrypt or decrypt a single filesystem block of file contents */
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index e84efc01512e4..76c64297ce187 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -163,12 +163,6 @@ struct fscrypt_info {
>         /* The actual crypto transform used for encryption and decryption */
>         struct crypto_skcipher *ci_ctfm;
>
> -       /*
> -        * Cipher for ESSIV IV generation.  Only set for CBC contents
> -        * encryption, otherwise is NULL.
> -        */
> -       struct crypto_cipher *ci_essiv_tfm;
> -
>         /*
>          * Encryption mode used for this inode.  It corresponds to either the
>          * contents or filenames encryption mode, depending on the inode type.
> @@ -444,7 +438,6 @@ struct fscrypt_mode {
>         int keysize;
>         int ivsize;
>         bool logged_impl_name;
> -       bool needs_essiv;
>  };
>
>  static inline bool
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index d71c2d6dd162a..8eb5a0e762ec6 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -8,15 +8,11 @@
>   * Heavily modified since then.
>   */
>
> -#include <crypto/aes.h>
> -#include <crypto/sha.h>
>  #include <crypto/skcipher.h>
>  #include <linux/key.h>
>
>  #include "fscrypt_private.h"
>
> -static struct crypto_shash *essiv_hash_tfm;
> -
>  static struct fscrypt_mode available_modes[] = {
>         [FSCRYPT_MODE_AES_256_XTS] = {
>                 .friendly_name = "AES-256-XTS",
> @@ -31,11 +27,10 @@ static struct fscrypt_mode available_modes[] = {
>                 .ivsize = 16,
>         },
>         [FSCRYPT_MODE_AES_128_CBC] = {
> -               .friendly_name = "AES-128-CBC",
> -               .cipher_str = "cbc(aes)",
> +               .friendly_name = "AES-128-CBC-ESSIV",
> +               .cipher_str = "essiv(cbc(aes),sha256)",
>                 .keysize = 16,
>                 .ivsize = 16,
> -               .needs_essiv = true,
>         },
>         [FSCRYPT_MODE_AES_128_CTS] = {
>                 .friendly_name = "AES-128-CTS-CBC",
> @@ -111,97 +106,16 @@ struct crypto_skcipher *fscrypt_allocate_skcipher(struct fscrypt_mode *mode,
>         return ERR_PTR(err);
>  }
>
> -static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
> -{
> -       struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);
> -
> -       /* init hash transform on demand */
> -       if (unlikely(!tfm)) {
> -               struct crypto_shash *prev_tfm;
> -
> -               tfm = crypto_alloc_shash("sha256", 0, 0);
> -               if (IS_ERR(tfm)) {
> -                       if (PTR_ERR(tfm) == -ENOENT) {
> -                               fscrypt_warn(NULL,
> -                                            "Missing crypto API support for SHA-256");
> -                               return -ENOPKG;
> -                       }
> -                       fscrypt_err(NULL,
> -                                   "Error allocating SHA-256 transform: %ld",
> -                                   PTR_ERR(tfm));
> -                       return PTR_ERR(tfm);
> -               }
> -               prev_tfm = cmpxchg(&essiv_hash_tfm, NULL, tfm);
> -               if (prev_tfm) {
> -                       crypto_free_shash(tfm);
> -                       tfm = prev_tfm;
> -               }
> -       }
> -
> -       {
> -               SHASH_DESC_ON_STACK(desc, tfm);
> -               desc->tfm = tfm;
> -
> -               return crypto_shash_digest(desc, key, keysize, salt);
> -       }
> -}
> -
> -static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
> -                               int keysize)
> -{
> -       int err;
> -       struct crypto_cipher *essiv_tfm;
> -       u8 salt[SHA256_DIGEST_SIZE];
> -
> -       if (WARN_ON(ci->ci_mode->ivsize != AES_BLOCK_SIZE))
> -               return -EINVAL;
> -
> -       essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
> -       if (IS_ERR(essiv_tfm))
> -               return PTR_ERR(essiv_tfm);
> -
> -       ci->ci_essiv_tfm = essiv_tfm;
> -
> -       err = derive_essiv_salt(raw_key, keysize, salt);
> -       if (err)
> -               goto out;
> -
> -       /*
> -        * Using SHA256 to derive the salt/key will result in AES-256 being
> -        * used for IV generation. File contents encryption will still use the
> -        * configured keysize (AES-128) nevertheless.
> -        */
> -       err = crypto_cipher_setkey(essiv_tfm, salt, sizeof(salt));
> -       if (err)
> -               goto out;
> -
> -out:
> -       memzero_explicit(salt, sizeof(salt));
> -       return err;
> -}
> -
> -/* Given the per-file key, set up the file's crypto transform object(s) */
> +/* Given the per-file key, set up the file's crypto transform object */
>  int fscrypt_set_derived_key(struct fscrypt_info *ci, const u8 *derived_key)
>  {
> -       struct fscrypt_mode *mode = ci->ci_mode;
> -       struct crypto_skcipher *ctfm;
> -       int err;
> -
> -       ctfm = fscrypt_allocate_skcipher(mode, derived_key, ci->ci_inode);
> -       if (IS_ERR(ctfm))
> -               return PTR_ERR(ctfm);
> +       struct crypto_skcipher *tfm;
>
> -       ci->ci_ctfm = ctfm;
> +       tfm = fscrypt_allocate_skcipher(ci->ci_mode, derived_key, ci->ci_inode);
> +       if (IS_ERR(tfm))
> +               return PTR_ERR(tfm);
>
> -       if (mode->needs_essiv) {
> -               err = init_essiv_generator(ci, derived_key, mode->keysize);
> -               if (err) {
> -                       fscrypt_warn(ci->ci_inode,
> -                                    "Error initializing ESSIV generator: %d",
> -                                    err);
> -                       return err;
> -               }
> -       }
> +       ci->ci_ctfm = tfm;
>         return 0;
>  }
>
> @@ -388,13 +302,11 @@ static void put_crypt_info(struct fscrypt_info *ci)
>         if (!ci)
>                 return;
>
> -       if (ci->ci_direct_key) {
> +       if (ci->ci_direct_key)
>                 fscrypt_put_direct_key(ci->ci_direct_key);
> -       } else if ((ci->ci_ctfm != NULL || ci->ci_essiv_tfm != NULL) &&
> -                  !fscrypt_is_direct_key_policy(&ci->ci_policy)) {
> +       else if (ci->ci_ctfm != NULL &&
> +                !fscrypt_is_direct_key_policy(&ci->ci_policy))
>                 crypto_free_skcipher(ci->ci_ctfm);
> -               crypto_free_cipher(ci->ci_essiv_tfm);
> -       }
>
>         key = ci->ci_master_key;
>         if (key) {
> diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
> index ad1a36c370c3f..5298ef22aa859 100644
> --- a/fs/crypto/keysetup_v1.c
> +++ b/fs/crypto/keysetup_v1.c
> @@ -270,10 +270,6 @@ static int setup_v1_file_key_direct(struct fscrypt_info *ci,
>                 return -EINVAL;
>         }
>
> -       /* ESSIV implies 16-byte IVs which implies !DIRECT_KEY */
> -       if (WARN_ON(mode->needs_essiv))
> -               return -EINVAL;
> -
>         dk = fscrypt_get_direct_key(ci, raw_master_key);
>         if (IS_ERR(dk))
>                 return PTR_ERR(dk);
> --
> 2.23.0.581.g78d2f28ef7-goog
>
Eric Biggers Oct. 21, 2019, 8:27 p.m. UTC | #2
On Wed, Oct 09, 2019 at 04:38:40PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Instead of open-coding the calculations for ESSIV handling, use an ESSIV
> skcipher which does all of this under the hood.  ESSIV was added to the
> crypto API in v5.4.
> 
> This is based on a patch from Ard Biesheuvel, but reworked to apply
> after all the fscrypt changes that went into v5.4.
> 
> Tested with 'kvm-xfstests -c ext4,f2fs -g encrypt', including the
> ciphertext verification tests for v1 and v2 encryption policies.
> 
> Originally-from: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---

Applied to fscrypt.git for 5.5.

- Eric

Patch
diff mbox series

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index 8a0700af95967..6ec459be3de16 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -308,8 +308,9 @@  If unsure, you should use the (AES-256-XTS, AES-256-CTS-CBC) pair.
 
 AES-128-CBC was added only for low-powered embedded devices with
 crypto accelerators such as CAAM or CESA that do not support XTS.  To
-use AES-128-CBC, CONFIG_CRYPTO_SHA256 (or another SHA-256
-implementation) must be enabled so that ESSIV can be used.
+use AES-128-CBC, CONFIG_CRYPTO_ESSIV and CONFIG_CRYPTO_SHA256 (or
+another SHA-256 implementation) must be enabled so that ESSIV can be
+used.
 
 Adiantum is a (primarily) stream cipher-based mode that is fast even
 on CPUs without dedicated crypto instructions.  It's also a true
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 32a7ad0098cc2..6bc3e4f1e657e 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -27,7 +27,6 @@ 
 #include <linux/ratelimit.h>
 #include <linux/dcache.h>
 #include <linux/namei.h>
-#include <crypto/aes.h>
 #include <crypto/skcipher.h>
 #include "fscrypt_private.h"
 
@@ -143,9 +142,6 @@  void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
 
 	if (fscrypt_is_direct_key_policy(&ci->ci_policy))
 		memcpy(iv->nonce, ci->ci_nonce, FS_KEY_DERIVATION_NONCE_SIZE);
-
-	if (ci->ci_essiv_tfm != NULL)
-		crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv->raw, iv->raw);
 }
 
 /* Encrypt or decrypt a single filesystem block of file contents */
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e84efc01512e4..76c64297ce187 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -163,12 +163,6 @@  struct fscrypt_info {
 	/* The actual crypto transform used for encryption and decryption */
 	struct crypto_skcipher *ci_ctfm;
 
-	/*
-	 * Cipher for ESSIV IV generation.  Only set for CBC contents
-	 * encryption, otherwise is NULL.
-	 */
-	struct crypto_cipher *ci_essiv_tfm;
-
 	/*
 	 * Encryption mode used for this inode.  It corresponds to either the
 	 * contents or filenames encryption mode, depending on the inode type.
@@ -444,7 +438,6 @@  struct fscrypt_mode {
 	int keysize;
 	int ivsize;
 	bool logged_impl_name;
-	bool needs_essiv;
 };
 
 static inline bool
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index d71c2d6dd162a..8eb5a0e762ec6 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -8,15 +8,11 @@ 
  * Heavily modified since then.
  */
 
-#include <crypto/aes.h>
-#include <crypto/sha.h>
 #include <crypto/skcipher.h>
 #include <linux/key.h>
 
 #include "fscrypt_private.h"
 
-static struct crypto_shash *essiv_hash_tfm;
-
 static struct fscrypt_mode available_modes[] = {
 	[FSCRYPT_MODE_AES_256_XTS] = {
 		.friendly_name = "AES-256-XTS",
@@ -31,11 +27,10 @@  static struct fscrypt_mode available_modes[] = {
 		.ivsize = 16,
 	},
 	[FSCRYPT_MODE_AES_128_CBC] = {
-		.friendly_name = "AES-128-CBC",
-		.cipher_str = "cbc(aes)",
+		.friendly_name = "AES-128-CBC-ESSIV",
+		.cipher_str = "essiv(cbc(aes),sha256)",
 		.keysize = 16,
 		.ivsize = 16,
-		.needs_essiv = true,
 	},
 	[FSCRYPT_MODE_AES_128_CTS] = {
 		.friendly_name = "AES-128-CTS-CBC",
@@ -111,97 +106,16 @@  struct crypto_skcipher *fscrypt_allocate_skcipher(struct fscrypt_mode *mode,
 	return ERR_PTR(err);
 }
 
-static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
-{
-	struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);
-
-	/* init hash transform on demand */
-	if (unlikely(!tfm)) {
-		struct crypto_shash *prev_tfm;
-
-		tfm = crypto_alloc_shash("sha256", 0, 0);
-		if (IS_ERR(tfm)) {
-			if (PTR_ERR(tfm) == -ENOENT) {
-				fscrypt_warn(NULL,
-					     "Missing crypto API support for SHA-256");
-				return -ENOPKG;
-			}
-			fscrypt_err(NULL,
-				    "Error allocating SHA-256 transform: %ld",
-				    PTR_ERR(tfm));
-			return PTR_ERR(tfm);
-		}
-		prev_tfm = cmpxchg(&essiv_hash_tfm, NULL, tfm);
-		if (prev_tfm) {
-			crypto_free_shash(tfm);
-			tfm = prev_tfm;
-		}
-	}
-
-	{
-		SHASH_DESC_ON_STACK(desc, tfm);
-		desc->tfm = tfm;
-
-		return crypto_shash_digest(desc, key, keysize, salt);
-	}
-}
-
-static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
-				int keysize)
-{
-	int err;
-	struct crypto_cipher *essiv_tfm;
-	u8 salt[SHA256_DIGEST_SIZE];
-
-	if (WARN_ON(ci->ci_mode->ivsize != AES_BLOCK_SIZE))
-		return -EINVAL;
-
-	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
-	if (IS_ERR(essiv_tfm))
-		return PTR_ERR(essiv_tfm);
-
-	ci->ci_essiv_tfm = essiv_tfm;
-
-	err = derive_essiv_salt(raw_key, keysize, salt);
-	if (err)
-		goto out;
-
-	/*
-	 * Using SHA256 to derive the salt/key will result in AES-256 being
-	 * used for IV generation. File contents encryption will still use the
-	 * configured keysize (AES-128) nevertheless.
-	 */
-	err = crypto_cipher_setkey(essiv_tfm, salt, sizeof(salt));
-	if (err)
-		goto out;
-
-out:
-	memzero_explicit(salt, sizeof(salt));
-	return err;
-}
-
-/* Given the per-file key, set up the file's crypto transform object(s) */
+/* Given the per-file key, set up the file's crypto transform object */
 int fscrypt_set_derived_key(struct fscrypt_info *ci, const u8 *derived_key)
 {
-	struct fscrypt_mode *mode = ci->ci_mode;
-	struct crypto_skcipher *ctfm;
-	int err;
-
-	ctfm = fscrypt_allocate_skcipher(mode, derived_key, ci->ci_inode);
-	if (IS_ERR(ctfm))
-		return PTR_ERR(ctfm);
+	struct crypto_skcipher *tfm;
 
-	ci->ci_ctfm = ctfm;
+	tfm = fscrypt_allocate_skcipher(ci->ci_mode, derived_key, ci->ci_inode);
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
 
-	if (mode->needs_essiv) {
-		err = init_essiv_generator(ci, derived_key, mode->keysize);
-		if (err) {
-			fscrypt_warn(ci->ci_inode,
-				     "Error initializing ESSIV generator: %d",
-				     err);
-			return err;
-		}
-	}
+	ci->ci_ctfm = tfm;
 	return 0;
 }
 
@@ -388,13 +302,11 @@  static void put_crypt_info(struct fscrypt_info *ci)
 	if (!ci)
 		return;
 
-	if (ci->ci_direct_key) {
+	if (ci->ci_direct_key)
 		fscrypt_put_direct_key(ci->ci_direct_key);
-	} else if ((ci->ci_ctfm != NULL || ci->ci_essiv_tfm != NULL) &&
-		   !fscrypt_is_direct_key_policy(&ci->ci_policy)) {
+	else if (ci->ci_ctfm != NULL &&
+		 !fscrypt_is_direct_key_policy(&ci->ci_policy))
 		crypto_free_skcipher(ci->ci_ctfm);
-		crypto_free_cipher(ci->ci_essiv_tfm);
-	}
 
 	key = ci->ci_master_key;
 	if (key) {
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index ad1a36c370c3f..5298ef22aa859 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -270,10 +270,6 @@  static int setup_v1_file_key_direct(struct fscrypt_info *ci,
 		return -EINVAL;
 	}
 
-	/* ESSIV implies 16-byte IVs which implies !DIRECT_KEY */
-	if (WARN_ON(mode->needs_essiv))
-		return -EINVAL;
-
 	dk = fscrypt_get_direct_key(ci, raw_master_key);
 	if (IS_ERR(dk))
 		return PTR_ERR(dk);