On 17 May 2017, at 20:08, Eric Biggers <ebiggers3@gmail.com> wrote:

Hi David, thanks for the update!

On Wed, May 17, 2017 at 01:21:04PM +0200, David Gstir wrote:

From: Daniel Walter <dwalter@sigma-star.at>

fscrypt provides facilities to use different encryption algorithms which
are selectable by userspace when setting the encryption policy. Currently,
only AES-256-XTS for file contents and AES-256-CBC-CTS for file names are
implemented. This is a clear case of kernel offers the mechanism and
userspace selects a policy. Similar to what dm-crypt and ecryptfs have.

This patch adds support for using AES-128-CBC for file contents and
AES-128-CBC-CTS for file name encryption. To mitigate watermarking
attacks, IVs are generated using the ESSIV algorithm. While AES-CBC is
actually slightly less secure than AES-XTS from a security point of view,
there is more widespread hardware support. Especially low-powered embedded
devices with crypto accelerators such as CAAM or CESA support only
AES-CBC-128 with an acceptable speed. Using AES-CBC gives us the acceptable
performance while still providing a moderate level of security for
persistent storage.

You covered this briefly in an email, but can you include more detail in the
commit message on the reasoning behind choosing AES-128 instead of AES-256?
Note that this is independent of the decision of CBC vs. XTS.

@@ -129,27 +136,37 @@ static int determine_cipher_type(struct fscrypt_info *ci, struct inode *inode,
 const char **cipher_str_ret, int *keysize_ret)
{
if (S_ISREG(inode->i_mode)) {
- if (ci->ci_data_mode == FS_ENCRYPTION_MODE_AES_256_XTS) {
+ switch (ci->ci_data_mode) {
+ case FS_ENCRYPTION_MODE_AES_256_XTS:
*cipher_str_ret = "xts(aes)";
*keysize_ret = FS_AES_256_XTS_KEY_SIZE;
return 0;
+ case FS_ENCRYPTION_MODE_AES_128_CBC:
+ *cipher_str_ret = "cbc(aes)";
+ *keysize_ret = FS_AES_128_CBC_KEY_SIZE;
+ return 0;
+ default:
+ pr_warn_once("fscrypto: unsupported contents encryption mode %d for inode %lu\n",
+      ci->ci_data_mode, inode->i_ino);
+ return -ENOKEY;
}
- pr_warn_once("fscrypto: unsupported contents encryption mode "
-      "%d for inode %lu\n",
-      ci->ci_data_mode, inode->i_ino);
- return -ENOKEY;
}

if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) {
- if (ci->ci_filename_mode == FS_ENCRYPTION_MODE_AES_256_CTS) {
+ switch (ci->ci_filename_mode) {
+ case FS_ENCRYPTION_MODE_AES_256_CTS:
*cipher_str_ret = "cts(cbc(aes))";
*keysize_ret = FS_AES_256_CTS_KEY_SIZE;
return 0;
+ case FS_ENCRYPTION_MODE_AES_128_CTS:
+ *cipher_str_ret = "cts(cbc(aes))";
+ *keysize_ret = FS_AES_128_CTS_KEY_SIZE;
+ return 0;
+ default:
+ pr_warn_once("fscrypto: unsupported filenames encryption mode %d for inode %lu\n",
+      ci->ci_filename_mode, inode->i_ino);
+ return -ENOKEY;
}
- pr_warn_once("fscrypto: unsupported filenames encryption mode "
-      "%d for inode %lu\n",
-      ci->ci_filename_mode, inode->i_ino);
- return -ENOKEY;
}

With the added call to fscrypt_valid_enc_modes() earlier, the warnings about
unsupported encryption modes are no longer reachable.  IMO, the
fscrypt_valid_enc_modes() check should be moved into this function, a proper
warning message added for it, and the redundant warnings removed.  Also now that
there will be more modes I think it would be appropriate to put the algorithm
names and key sizes in a table, to avoid the ugly switch statements.  

+ int err;
+
+ /* init hash transform on demand */
+ if (unlikely(essiv_hash_tfm == NULL)) {
+ mutex_lock(&essiv_hash_lock);
+ if (essiv_hash_tfm == NULL) {
+ essiv_hash_tfm = crypto_alloc_shash("sha256", 0, 0);
+ if (IS_ERR(essiv_hash_tfm)) {
+ pr_warn_ratelimited("fscrypt: error allocating SHA-256 transform: %ld\n",
+     PTR_ERR(essiv_hash_tfm));
+ err = PTR_ERR(essiv_hash_tfm);
+ essiv_hash_tfm = NULL;
+ mutex_unlock(&essiv_hash_lock);
+ return err;
+ }
+ }
+ mutex_unlock(&essiv_hash_lock);
+ }

There is a bug here: a thread can set essiv_hash_tfm to an ERR_PTR(), and
another thread can use it before it's set back to NULL.  

+ err = crypto_cipher_setkey(essiv_tfm, salt, SHA256_DIGEST_SIZE);
+ if (err)
+ goto out;

sizeof(salt) instead of hardcoding SHA256_DIGEST_SIZE.

I think there should also be a brief comment explaining that the ESSIV cipher
uses AES-256 so that its key size matches the size of the hash, even though the
"real" encryption may use AES-128.

+void fscrypt_essiv_cleanup(void)
+{
+ crypto_free_shash(essiv_hash_tfm);
+ essiv_hash_tfm = NULL;
+}

This is called from fscrypt_destroy(), which is a little weird because
fscrypt_destroy() is meant to clean up only from "fscrypt_initialize()", which
only allocates certain things.  I think it should be called from
"fscrypt_exit()" instead.  Then you could also add the __exit annotation, and
remove setting essiv_hash_tfm to NULL which would clearly be unnecessary.