On Thu, 10 Oct 2019 at 01:39, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
Thanks for picking this up.
Acked-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
[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
>
