[PATCH] fscrypto: remove unnecessary includes

[PATCH] fscrypto: remove unnecessary includes

[Eric Biggers]

This patch removes some #includes that are clearly not needed, such as a
reference to ecryptfs, which is unrelated to the new filesystem
encryption code.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/crypto.c  | 1 -
 fs/crypto/fname.c   | 2 --
 fs/crypto/keyinfo.c | 3 ---
 3 files changed, 6 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index c502c11..7c39eab 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -28,7 +28,6 @@
 #include <linux/dcache.h>
 #include <linux/namei.h>
 #include <linux/fscrypto.h>
-#include <linux/ecryptfs.h>
 
 static unsigned int num_prealloc_crypto_pages = 32;
 static unsigned int num_prealloc_crypto_ctxs = 128;
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 5d6d491..3108806 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -10,8 +10,6 @@
  * This has not yet undergone a rigorous security audit.
  */
 
-#include <keys/encrypted-type.h>
-#include <keys/user-type.h>
 #include <linux/scatterlist.h>
 #include <linux/ratelimit.h>
 #include <linux/fscrypto.h>
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 1ac263e..58b1c82 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -8,11 +8,8 @@
  * Written by Michael Halcrow, Ildar Muslukhov, and Uday Savagaonkar, 2015.
  */
 
-#include <keys/encrypted-type.h>
 #include <keys/user-type.h>
-#include <linux/random.h>
 #include <linux/scatterlist.h>
-#include <uapi/linux/keyctl.h>
 #include <linux/fscrypto.h>
 
 static void derive_crypt_complete(struct crypto_async_request *req, int rc)
-- 
2.8.0.rc3.226.g39d4020

[PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

[Eric Biggers]

This makes the return value match the comment.  Previously it would
actually return 0 if encryption was successful.  No callers currently
care, but this change should reduce the chance of future bugs.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 3108806..7f3239c 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -103,12 +103,13 @@ static int fname_encrypt(struct inode *inode,
 	}
 	kfree(alloc_buf);
 	skcipher_request_free(req);
-	if (res < 0)
+	if (res < 0) {
 		printk_ratelimited(KERN_ERR
 				"%s: Error (error code %d)\n", __func__, res);

Re: [PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2848 bytes --]

On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@google.com> wrote:
> 
> This makes the return value match the comment.  Previously it would
> actually return 0 if encryption was successful.  No callers currently
> care, but this change should reduce the chance of future bugs.

This may be introducing a subtle bug in fscrypt_fname_usr_to_disk(), since
that function returns the status from fname_encrypt() directly and now it
returns the name length instead of 0 on success:

int fscrypt_fname_usr_to_disk(struct inode *inode,
                        const struct qstr *iname,
                        struct fscrypt_str *oname)
{
        if (fscrypt_is_dot_dotdot(iname)) {
                oname->name[0] = '.';
                oname->name[iname->len - 1] = '.';
                oname->len = iname->len;
                return oname->len;
        }
        if (inode->i_crypt_info)
                return fname_encrypt(inode, iname, oname);
        /*
         * Without a proper key, a user is not allowed to modify the filenames
         * in a directory. Consequently, a user space name cannot be mapped to
         * a disk-space name
         */
        return -EACCES;
}

This percolates further up to some of the callers, but in the cases that I
saw the check is "if (err < 0)" and the positive value is either ignored
or overwritten before being returned further up the call chain.  However,
that could be easily missed in the future and somewhere up the call chain
doing "if (rc)" would suddenly start to fail.

Since both "struct fscrypt_str" and "struct qstr" already hold the length
I don't think there is any benefit to returning the length to the caller.
Since (IMHO) this creates a non-trivial chance of introducing bugs in the
future it makes more sense to just change the function comment to match the
actual behaviour.

Cheers, Andreas

> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> fs/crypto/fname.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 3108806..7f3239c 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -103,12 +103,13 @@ static int fname_encrypt(struct inode *inode,
> 	}
> 	kfree(alloc_buf);
> 	skcipher_request_free(req);
> -	if (res < 0)
> +	if (res < 0) {
> 		printk_ratelimited(KERN_ERR
> 				"%s: Error (error code %d)\n", __func__, res);
> -
> +		return res;
> +	}
> 	oname->len = ciphertext_len;
> -	return res;
> +	return ciphertext_len;
> }
> 
> /*
> --
> 2.8.0.rc3.226.g39d4020
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

[Eric Biggers]

On Wed, Sep 14, 2016 at 03:37:01PM -0600, Andreas Dilger wrote:
> On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@google.com> wrote:
> > 
> > This makes the return value match the comment.  Previously it would
> > actually return 0 if encryption was successful.  No callers currently
> > care, but this change should reduce the chance of future bugs.
> 
> This may be introducing a subtle bug in fscrypt_fname_usr_to_disk(), since
> that function returns the status from fname_encrypt() directly and now it
> returns the name length instead of 0 on success:
> 

fscrypt_fname_usr_to_disk() already returned a length in the "." and ".." cases.
So any caller which assumed it returned 0 on success would have already been
buggy.  Fortunately, there aren't any such callers currently.

> 
> This percolates further up to some of the callers, but in the cases that I
> saw the check is "if (err < 0)" and the positive value is either ignored
> or overwritten before being returned further up the call chain.  However,
> that could be easily missed in the future and somewhere up the call chain
> doing "if (rc)" would suddenly start to fail.
> 
> Since both "struct fscrypt_str" and "struct qstr" already hold the length
> I don't think there is any benefit to returning the length to the caller.
> Since (IMHO) this creates a non-trivial chance of introducing bugs in the
> future it makes more sense to just change the function comment to match the
> actual behaviour.
> 

I agree that the return value is redundant and somewhat error prone.  However,
this style is already being used for fscrypt_fname_disk_to_usr(),
fscrypt_fname_usr_to_disk(), and fname_decrypt().  My patch was primarily
intended to make things more consistent by updating fname_encrypt(), which was
the odd one out.  If you'd prefer, I can instead do a patch to make all these
related functions return 0 on success, rather than a length.  That would be a
somewhat larger patch.

Eric

Re: [PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

[Eric Biggers]

On Wed, Sep 14, 2016 at 02:57:04PM -0700, Eric Biggers wrote:
> I agree that the return value is redundant and somewhat error prone.  However,
> this style is already being used for fscrypt_fname_disk_to_usr(),
> fscrypt_fname_usr_to_disk(), and fname_decrypt().  My patch was primarily
> intended to make things more consistent by updating fname_encrypt(), which was
> the odd one out.  If you'd prefer, I can instead do a patch to make all these
> related functions return 0 on success, rather than a length.  That would be a
> somewhat larger patch.
> 

To see more concretely what it looks like, I went ahead and wrote the "make the
functions return 0" version of the patch.  I'm sending it to be considered as
well.

In theory I think it's better, though it's a larger patch.

[PATCH] fscrypto: rename completion callbacks to reflect usage

[Eric Biggers]

fscrypt_complete() was used only for data pages, not for all
encryption/decryption.  Rename it to page_crypt_complete().

dir_crypt_complete() was used for filename encryption/decryption for
both directory entries and symbolic links.  Rename it to
fname_crypt_complete().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/crypto.c | 10 +++++-----
 fs/crypto/fname.c  | 10 ++++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 7c39eab..61057b7d 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -127,11 +127,11 @@ struct fscrypt_ctx *fscrypt_get_ctx(struct inode *inode, gfp_t gfp_flags)
 EXPORT_SYMBOL(fscrypt_get_ctx);
 
 /**
- * fscrypt_complete() - The completion callback for page encryption
- * @req: The asynchronous encryption request context
- * @res: The result of the encryption operation
+ * page_crypt_complete() - completion callback for page crypto
+ * @req: The asynchronous cipher request context
+ * @res: The result of the cipher operation
  */
-static void fscrypt_complete(struct crypto_async_request *req, int res)
+static void page_crypt_complete(struct crypto_async_request *req, int res)
 {
 	struct fscrypt_completion_result *ecr = req->data;
 
@@ -169,7 +169,7 @@ static int do_page_crypto(struct inode *inode,
 
 	skcipher_request_set_callback(
 		req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
-		fscrypt_complete, &ecr);
+		page_crypt_complete, &ecr);
 
 	BUILD_BUG_ON(FS_XTS_TWEAK_SIZE < sizeof(index));
 	memcpy(xts_tweak, &index, sizeof(index));
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 7f3239c..db8f798 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -20,9 +20,11 @@ static u32 size_round_up(size_t size, size_t blksize)
 }
 
 /**
- * dir_crypt_complete() -
+ * fname_crypt_complete() - completion callback for filename crypto
+ * @req: The asynchronous cipher request context
+ * @res: The result of the cipher operation
  */
-static void dir_crypt_complete(struct crypto_async_request *req, int res)
+static void fname_crypt_complete(struct crypto_async_request *req, int res)
 {
 	struct fscrypt_completion_result *ecr = req->data;
 
@@ -82,7 +84,7 @@ static int fname_encrypt(struct inode *inode,
 	}
 	skcipher_request_set_callback(req,
 			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
-			dir_crypt_complete, &ecr);
+			fname_crypt_complete, &ecr);
 
 	/* Copy the input */
 	memcpy(workbuf, iname->name, iname->len);
@@ -145,7 +147,7 @@ static int fname_decrypt(struct inode *inode,
 	}
 	skcipher_request_set_callback(req,
 		CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
-		dir_crypt_complete, &ecr);
+		fname_crypt_complete, &ecr);
 
 	/* Initialize IV */
 	memset(iv, 0, FS_CRYPTO_BLOCK_SIZE);
-- 
2.8.0.rc3.226.g39d4020

Re: fscrypto: rename completion callbacks to reflect usage

[Theodore Ts'o]

On Wed, Sep 14, 2016 at 01:57:14PM -0700, Eric Biggers wrote:
> fscrypt_complete() was used only for data pages, not for all
> encryption/decryption.  Rename it to page_crypt_complete().
> 
> dir_crypt_complete() was used for filename encryption/decryption for
> both directory entries and symbolic links.  Rename it to
> fname_crypt_complete().
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

						- Ted

Re: fscrypto: remove unnecessary includes

[Theodore Ts'o]

On Wed, Sep 14, 2016 at 01:57:12PM -0700, Eric Biggers wrote:
> This patch removes some #includes that are clearly not needed, such as a
> reference to ecryptfs, which is unrelated to the new filesystem
> encryption code.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted