[PATCH] fscrypt: use AES library for v1 key derivation

[Eric Biggers <ebiggers@kernel.org>]

Convert the implementation of the v1 (original / deprecated) fscrypt
per-file key derivation algorithm to use the AES library instead of an
"ecb(aes)" crypto_skcipher.  This is much simpler.

While the AES library doesn't support AES-ECB directly yet, we can still
simply call aes_encrypt() in a loop.  While that doesn't explicitly
parallelize the AES encryptions, it doesn't really matter in this case,
where a new key is used each time and only 16 to 64 bytes are encrypted.

In fact, a quick benchmark (AMD Ryzen 9 9950X) shows that this commit
actually greatly improves performance, from ~7000 cycles per key derived
to ~1500.  The times don't differ much between 32 bytes and 64 bytes
either, so clearly the bottleneck is API stuff and key expansion.

Granted, performance of the v1 key derivation is no longer very
relevant: most users have moved onto v2 encryption policies.  The v2 key
derivation uses HKDF-SHA512 (which is ~3500 cycles on the same CPU).

Still, it's nice that the simpler solution is much faster as well.

Compatibility verified with xfstests generic/548.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---

This patch is targeting fscrypt/for-next

 fs/crypto/Kconfig       |  2 +-
 fs/crypto/keysetup_v1.c | 87 +++++++++++++----------------------------
 2 files changed, 29 insertions(+), 60 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 464b54610fd3..983d8ad1f417 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -1,10 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config FS_ENCRYPTION
 	bool "FS Encryption (Per-file encryption)"
 	select CRYPTO
 	select CRYPTO_SKCIPHER
+	select CRYPTO_LIB_AES
 	select CRYPTO_LIB_SHA256
 	select CRYPTO_LIB_SHA512
 	select KEYS
 	help
 	  Enable encryption of files and directories.  This
@@ -28,11 +29,10 @@ config FS_ENCRYPTION
 config FS_ENCRYPTION_ALGS
 	tristate
 	select CRYPTO_AES
 	select CRYPTO_CBC
 	select CRYPTO_CTS
-	select CRYPTO_ECB
 	select CRYPTO_XTS
 
 config FS_ENCRYPTION_INLINE_CRYPT
 	bool "Enable fscrypt to use inline crypto"
 	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 3d673c36b678..e6e527c73f16 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -18,64 +18,21 @@
  * - Handling policies with the DIRECT_KEY flag set using a master key table
  *   (rather than the new method of implementing DIRECT_KEY with per-mode keys
  *    managed alongside the master keys in the filesystem-level keyring)
  */
 
-#include <crypto/skcipher.h>
+#include <crypto/aes.h>
 #include <crypto/utils.h>
 #include <keys/user-type.h>
 #include <linux/hashtable.h>
-#include <linux/scatterlist.h>
 
 #include "fscrypt_private.h"
 
 /* Table of keys referenced by DIRECT_KEY policies */
 static DEFINE_HASHTABLE(fscrypt_direct_keys, 6); /* 6 bits = 64 buckets */
 static DEFINE_SPINLOCK(fscrypt_direct_keys_lock);
 
-/*
- * v1 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 with sufficient entropy for each inode.  However, it's
- * nonstandard, non-extensible, doesn't evenly distribute the entropy from the
- * master key, and is trivially reversible: an attacker who compromises a
- * derived key can "decrypt" it to get back to the master key, then derive any
- * other key.  For all new code, use HKDF instead.
- *
- * The master key must be at least as long as the derived key.  If the master
- * key is longer, then only the first 'derived_keysize' bytes are used.
- */
-static int derive_key_aes(const u8 *master_key,
-			  const u8 nonce[FSCRYPT_FILE_NONCE_SIZE],
-			  u8 *derived_key, unsigned int derived_keysize)
-{
-	struct crypto_sync_skcipher *tfm;
-	int err;
-
-	tfm = crypto_alloc_sync_skcipher("ecb(aes)", 0, FSCRYPT_CRYPTOAPI_MASK);
-	if (IS_ERR(tfm))
-		return PTR_ERR(tfm);
-
-	err = crypto_sync_skcipher_setkey(tfm, nonce, FSCRYPT_FILE_NONCE_SIZE);
-	if (err == 0) {
-		SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
-		struct scatterlist src_sg, dst_sg;
-
-		skcipher_request_set_callback(req,
-					      CRYPTO_TFM_REQ_MAY_BACKLOG |
-						      CRYPTO_TFM_REQ_MAY_SLEEP,
-					      NULL, NULL);
-		sg_init_one(&src_sg, master_key, derived_keysize);
-		sg_init_one(&dst_sg, derived_key, derived_keysize);
-		skcipher_request_set_crypt(req, &src_sg, &dst_sg,
-					   derived_keysize, NULL);
-		err = crypto_skcipher_encrypt(req);
-	}
-	crypto_free_sync_skcipher(tfm);
-	return err;
-}
-
 /*
  * Search the current task's subscribed keyrings for a "logon" key with
  * description prefix:descriptor, and if found acquire a read lock on it and
  * return a pointer to its validated payload in *payload_ret.
  */
@@ -253,33 +210,45 @@ static int setup_v1_file_key_direct(struct fscrypt_inode_info *ci,
 	ci->ci_direct_key = dk;
 	ci->ci_enc_key = dk->dk_key;
 	return 0;
 }
 
-/* v1 policy, !DIRECT_KEY: derive the file's encryption key */
+/*
+ * v1 policy, !DIRECT_KEY: derive the file's encryption key.
+ *
+ * The v1 key derivation function generates the derived key by encrypting the
+ * master key with AES-128-ECB using the file's nonce as the AES key.  This
+ * provides a unique derived key with sufficient entropy for each inode.
+ * However, it's nonstandard, non-extensible, doesn't evenly distribute the
+ * entropy from the master key, and is trivially reversible: an attacker who
+ * compromises a derived key can "decrypt" it to get back to the master key,
+ * then derive any other key.  For all new code, use HKDF instead.
+ *
+ * The master key must be at least as long as the derived key.  If the master
+ * key is longer, then only the first ci->ci_mode->keysize bytes are used.
+ */
 static int setup_v1_file_key_derived(struct fscrypt_inode_info *ci,
 				     const u8 *raw_master_key)
 {
-	u8 *derived_key;
+	const unsigned int derived_keysize = ci->ci_mode->keysize;
+	u8 derived_key[FSCRYPT_MAX_RAW_KEY_SIZE];
+	struct aes_enckey aes;
 	int err;
 
-	/*
-	 * This cannot be a stack buffer because it will be passed to the
-	 * scatterlist crypto API during derive_key_aes().
-	 */
-	derived_key = kmalloc(ci->ci_mode->keysize, GFP_KERNEL);
-	if (!derived_key)
-		return -ENOMEM;
+	if (WARN_ON_ONCE(derived_keysize > FSCRYPT_MAX_RAW_KEY_SIZE ||
+			 derived_keysize % AES_BLOCK_SIZE != 0))
+		return -EINVAL;
 
-	err = derive_key_aes(raw_master_key, ci->ci_nonce,
-			     derived_key, ci->ci_mode->keysize);
-	if (err)
-		goto out;
+	static_assert(FSCRYPT_FILE_NONCE_SIZE == AES_KEYSIZE_128);
+	aes_prepareenckey(&aes, ci->ci_nonce, FSCRYPT_FILE_NONCE_SIZE);
+	for (unsigned int i = 0; i < derived_keysize; i += AES_BLOCK_SIZE)
+		aes_encrypt(&aes, &derived_key[i], &raw_master_key[i]);
 
 	err = fscrypt_set_per_file_enc_key(ci, derived_key);
-out:
-	kfree_sensitive(derived_key);
+
+	memzero_explicit(derived_key, derived_keysize);
+	/* No need to zeroize 'aes', as its key is not secret. */
 	return err;
 }
 
 int fscrypt_setup_v1_file_key(struct fscrypt_inode_info *ci,
 			      const u8 *raw_master_key)

base-commit: 4377a22d84f726f0a650927edf75cdc0698baf06
-- 
2.53.0