| Message ID | 20190812145324.27090-3-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: switch to crypto API for ESSIV generation | expand |
On Mon, Aug 12, 2019 at 05:53:19PM +0300, Ard Biesheuvel wrote: > Instead of open coding the calculations for ESSIV handling, use a > ESSIV skcipher which does all of this under the hood. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> This looks fine (except for one comment below), but this heavily conflicts with the fscrypt patches planned for v5.4. So I suggest moving this to the end of the series and having Herbert take only 1-6, and I'll apply this one to the fscrypt tree later. Thanks! > --- > fs/crypto/Kconfig | 1 + > fs/crypto/crypto.c | 5 -- > fs/crypto/fscrypt_private.h | 9 -- > fs/crypto/keyinfo.c | 92 +------------------- > 4 files changed, 4 insertions(+), 103 deletions(-) > > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig > index 5fdf24877c17..6f3d59b880b7 100644 > --- a/fs/crypto/Kconfig > +++ b/fs/crypto/Kconfig > @@ -5,6 +5,7 @@ config FS_ENCRYPTION > select CRYPTO_AES > select CRYPTO_CBC > select CRYPTO_ECB > + select CRYPTO_ESSIV > select CRYPTO_XTS > select CRYPTO_CTS > select KEYS In v5.3 I removed the 'select CRYPTO_SHA256', so now ESSIV shouldn't be selected here either. Instead we should just update the documentation: diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst index 82efa41b0e6c02..a1e2ab12a99943 100644 --- a/Documentation/filesystems/fscrypt.rst +++ b/Documentation/filesystems/fscrypt.rst @@ -193,7 +193,8 @@ 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. +implementation) and CONFIG_CRYPTO_ESSIV 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
On Mon, 12 Aug 2019 at 22:47, Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Aug 12, 2019 at 05:53:19PM +0300, Ard Biesheuvel wrote: > > Instead of open coding the calculations for ESSIV handling, use a > > ESSIV skcipher which does all of this under the hood. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > This looks fine (except for one comment below), but this heavily conflicts with > the fscrypt patches planned for v5.4. So I suggest moving this to the end of > the series and having Herbert take only 1-6, and I'll apply this one to the > fscrypt tree later. > I think the same applies to dm-crypt: at least patch #7 cannot be applied until my eboiv patch is applied there as well, but [Milan should confirm] I'd expect them to prefer taking those patches via the dm tree anyway. Herbert, what would you prefer: - taking a pull request from a [signed] tag based on v4.3-rc1 that contains patches #1, #4, #5 and #6, allowing Eric and Milan/Mike to merge it as well, and apply the respective fscrypt and dm-crypt changes on top - just take patches #1, #4, #5 and #6 as usual, and let the fscrypt and dm-crypt changes be reposted to the respective lists during the next cycle > > > --- > > fs/crypto/Kconfig | 1 + > > fs/crypto/crypto.c | 5 -- > > fs/crypto/fscrypt_private.h | 9 -- > > fs/crypto/keyinfo.c | 92 +------------------- > > 4 files changed, 4 insertions(+), 103 deletions(-) > > > > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig > > index 5fdf24877c17..6f3d59b880b7 100644 > > --- a/fs/crypto/Kconfig > > +++ b/fs/crypto/Kconfig > > @@ -5,6 +5,7 @@ config FS_ENCRYPTION > > select CRYPTO_AES > > select CRYPTO_CBC > > select CRYPTO_ECB > > + select CRYPTO_ESSIV > > select CRYPTO_XTS > > select CRYPTO_CTS > > select KEYS > > In v5.3 I removed the 'select CRYPTO_SHA256', so now ESSIV shouldn't be selected > here either. Instead we should just update the documentation: > > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst > index 82efa41b0e6c02..a1e2ab12a99943 100644 > --- a/Documentation/filesystems/fscrypt.rst > +++ b/Documentation/filesystems/fscrypt.rst > @@ -193,7 +193,8 @@ 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. > +implementation) and CONFIG_CRYPTO_ESSIV 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
On Tue, Aug 13, 2019 at 08:09:41AM +0300, Ard Biesheuvel wrote: > On Mon, 12 Aug 2019 at 22:47, Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Mon, Aug 12, 2019 at 05:53:19PM +0300, Ard Biesheuvel wrote: > > > Instead of open coding the calculations for ESSIV handling, use a > > > ESSIV skcipher which does all of this under the hood. > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > This looks fine (except for one comment below), but this heavily conflicts with > > the fscrypt patches planned for v5.4. So I suggest moving this to the end of > > the series and having Herbert take only 1-6, and I'll apply this one to the > > fscrypt tree later. > > > > I think the same applies to dm-crypt: at least patch #7 cannot be > applied until my eboiv patch is applied there as well, but [Milan > should confirm] I'd expect them to prefer taking those patches via the > dm tree anyway. > > Herbert, what would you prefer: > - taking a pull request from a [signed] tag based on v4.3-rc1 that > contains patches #1, #4, #5 and #6, allowing Eric and Milan/Mike to > merge it as well, and apply the respective fscrypt and dm-crypt > changes on top > - just take patches #1, #4, #5 and #6 as usual, and let the fscrypt > and dm-crypt changes be reposted to the respective lists during the > next cycle > FWIW I'd much prefer the second option, to minimize the number of special things that Linus will have to consider or deal with. (There's also going to be a conflict between the fscrypt and keyrings trees.) I'd be glad to take the fscrypt patch for 5.5, if the essiv template is added in 5.4. - Eric
On Tue, 13 Aug 2019 at 21:00, Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Aug 13, 2019 at 08:09:41AM +0300, Ard Biesheuvel wrote: > > On Mon, 12 Aug 2019 at 22:47, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Mon, Aug 12, 2019 at 05:53:19PM +0300, Ard Biesheuvel wrote: > > > > Instead of open coding the calculations for ESSIV handling, use a > > > > ESSIV skcipher which does all of this under the hood. > > > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > > > This looks fine (except for one comment below), but this heavily conflicts with > > > the fscrypt patches planned for v5.4. So I suggest moving this to the end of > > > the series and having Herbert take only 1-6, and I'll apply this one to the > > > fscrypt tree later. > > > > > > > I think the same applies to dm-crypt: at least patch #7 cannot be > > applied until my eboiv patch is applied there as well, but [Milan > > should confirm] I'd expect them to prefer taking those patches via the > > dm tree anyway. > > > > Herbert, what would you prefer: > > - taking a pull request from a [signed] tag based on v4.3-rc1 that > > contains patches #1, #4, #5 and #6, allowing Eric and Milan/Mike to > > merge it as well, and apply the respective fscrypt and dm-crypt > > changes on top > > - just take patches #1, #4, #5 and #6 as usual, and let the fscrypt > > and dm-crypt changes be reposted to the respective lists during the > > next cycle > > > > FWIW I'd much prefer the second option, to minimize the number of special things > that Linus will have to consider or deal with. (There's also going to be a > conflict between the fscrypt and keyrings trees.) I'd be glad to take the > fscrypt patch for 5.5, if the essiv template is added in 5.4. > Works for me. I'll respin with the dm-crypt and fscrypt patches omitted (and the minor fixes you suggested applied).
diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig index 5fdf24877c17..6f3d59b880b7 100644 --- a/fs/crypto/Kconfig +++ b/fs/crypto/Kconfig @@ -5,6 +5,7 @@ config FS_ENCRYPTION select CRYPTO_AES select CRYPTO_CBC select CRYPTO_ECB + select CRYPTO_ESSIV select CRYPTO_XTS select CRYPTO_CTS select KEYS diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c index 45c3d0427fb2..fd13231c5ff6 100644 --- a/fs/crypto/crypto.c +++ b/fs/crypto/crypto.c @@ -143,9 +143,6 @@ void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num, if (ci->ci_flags & FS_POLICY_FLAG_DIRECT_KEY) 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 */ @@ -523,8 +520,6 @@ static void __exit fscrypt_exit(void) destroy_workqueue(fscrypt_read_workqueue); kmem_cache_destroy(fscrypt_ctx_cachep); kmem_cache_destroy(fscrypt_info_cachep); - - fscrypt_essiv_cleanup(); } module_exit(fscrypt_exit); diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 8978eec9d766..2fc6f0bd2d13 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -61,12 +61,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 * ci_data_mode or ci_filename_mode, depending on the inode type. @@ -163,9 +157,6 @@ struct fscrypt_mode { int keysize; int ivsize; bool logged_impl_name; - bool needs_essiv; }; -extern void __exit fscrypt_essiv_cleanup(void); - #endif /* _FSCRYPT_PRIVATE_H */ diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c index 207ebed918c1..80924a0f72ca 100644 --- a/fs/crypto/keyinfo.c +++ b/fs/crypto/keyinfo.c @@ -14,12 +14,9 @@ #include <linux/scatterlist.h> #include <crypto/aes.h> #include <crypto/algapi.h> -#include <crypto/sha.h> #include <crypto/skcipher.h> #include "fscrypt_private.h" -static struct crypto_shash *essiv_hash_tfm; - /* Table of keys referenced by FS_POLICY_FLAG_DIRECT_KEY policies */ static DEFINE_HASHTABLE(fscrypt_master_keys, 6); /* 6 bits = 64 buckets */ static DEFINE_SPINLOCK(fscrypt_master_keys_lock); @@ -143,10 +140,9 @@ static struct fscrypt_mode available_modes[] = { }, [FS_ENCRYPTION_MODE_AES_128_CBC] = { .friendly_name = "AES-128-CBC", - .cipher_str = "cbc(aes)", + .cipher_str = "essiv(cbc(aes),sha256)", .keysize = 16, .ivsize = 16, - .needs_essiv = true, }, [FS_ENCRYPTION_MODE_AES_128_CTS] = { .friendly_name = "AES-128-CTS-CBC", @@ -376,72 +372,6 @@ fscrypt_get_master_key(const struct fscrypt_info *ci, 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)) { - fscrypt_warn(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]; - - 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; -} - -void __exit fscrypt_essiv_cleanup(void) -{ - crypto_free_shash(essiv_hash_tfm); -} - /* * Given the encryption mode and key (normally the derived key, but for * FS_POLICY_FLAG_DIRECT_KEY mode it's the master key), set up the inode's @@ -453,7 +383,6 @@ static int setup_crypto_transform(struct fscrypt_info *ci, { struct fscrypt_master_key *mk; struct crypto_skcipher *ctfm; - int err; if (ci->ci_flags & FS_POLICY_FLAG_DIRECT_KEY) { mk = fscrypt_get_master_key(ci, mode, raw_key, inode); @@ -469,19 +398,6 @@ static int setup_crypto_transform(struct fscrypt_info *ci, ci->ci_master_key = mk; ci->ci_ctfm = ctfm; - if (mode->needs_essiv) { - /* ESSIV implies 16-byte IVs which implies !DIRECT_KEY */ - WARN_ON(mode->ivsize != AES_BLOCK_SIZE); - WARN_ON(ci->ci_flags & FS_POLICY_FLAG_DIRECT_KEY); - - err = init_essiv_generator(ci, raw_key, mode->keysize); - if (err) { - fscrypt_warn(inode->i_sb, - "error initializing ESSIV generator for inode %lu: %d", - inode->i_ino, err); - return err; - } - } return 0; } @@ -490,12 +406,10 @@ static void put_crypt_info(struct fscrypt_info *ci) if (!ci) return; - if (ci->ci_master_key) { + if (ci->ci_master_key) put_master_key(ci->ci_master_key); - } else { + else crypto_free_skcipher(ci->ci_ctfm); - crypto_free_cipher(ci->ci_essiv_tfm); - } kmem_cache_free(fscrypt_info_cachep, ci); }
Instead of open coding the calculations for ESSIV handling, use a ESSIV skcipher which does all of this under the hood. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- fs/crypto/Kconfig | 1 + fs/crypto/crypto.c | 5 -- fs/crypto/fscrypt_private.h | 9 -- fs/crypto/keyinfo.c | 92 +------------------- 4 files changed, 4 insertions(+), 103 deletions(-)