[RFC PATCH 0/5] fscrypt error code cleanup

[RFC PATCH 0/5] fscrypt error code cleanup

[Eric Biggers]

As suggested by Richard Weinberger, we need to agree on and document a clear set
of fscrypt-related error codes.

I identified several cases where the error codes are not consistent between or
within filesystems, or are ambiguous with other errors.  This patchset proposes
several changes to resolve these problems.

I believe we do still have some flexibility to make changes to these error codes
since there are not many different users of filesystem-level encryption yet, and
depending on these specific errors would be unusual.  There have also already
been cases where some of these error codes have changed before.

Currently this patchset depends on the following patch being applied first:
"fscrypto: move ioctl processing more fully into common code"

Eric Biggers (5):
  fscrypt: use ENOKEY when file cannot be created w/o key
  fscrypt: use ENOTDIR when setting encryption policy on nondirectory
  fscrypt: use EEXIST when file already uses different policy
  fscrypt: remove user-triggerable warning messages
  fscrypt: pass up error codes from ->get_context()

 fs/crypto/fname.c  |  4 +--
 fs/crypto/policy.c | 73 ++++++++++++++++++++----------------------------------
 fs/ext4/ialloc.c   |  2 +-
 fs/ext4/namei.c    |  4 ++-
 fs/f2fs/dir.c      |  5 +++-
 fs/f2fs/namei.c    |  4 +--
 6 files changed, 39 insertions(+), 53 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[RFC PATCH 1/5] fscrypt: use ENOKEY when file cannot be created w/o key

[Eric Biggers]

As part of an effort to clean up fscrypt-related error codes, make
attempting to create a file in an encrypted directory that hasn't been
"unlocked" fail with ENOKEY.  Previously, several error codes were used
for this case, including ENOENT, EACCES, and EPERM, and they were not
consistent between and within filesystems.  ENOKEY is a better choice
because it expresses that the failure is due to lacking the encryption
key.  It also matches the error code returned when trying to open an
encrypted regular file without the key.

I am not aware of any users who might be relying on the previous
inconsistent error codes, which were never documented anywhere.

This failure case will be exercised by an xfstest.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c | 4 ++--
 fs/ext4/ialloc.c  | 2 +-
 fs/ext4/namei.c   | 4 +++-
 fs/f2fs/dir.c     | 5 ++++-
 fs/f2fs/namei.c   | 4 ++--
 5 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6b45d9c..c902b1a 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -332,7 +332,7 @@ int fscrypt_fname_usr_to_disk(struct inode *inode,
 	 * in a directory. Consequently, a user space name cannot be mapped to
 	 * a disk-space name
 	 */
-	return -EACCES;
+	return -ENOKEY;
 }
 EXPORT_SYMBOL(fscrypt_fname_usr_to_disk);
 
@@ -367,7 +367,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 		return 0;
 	}
 	if (!lookup)
-		return -EACCES;
+		return -ENOKEY;
 
 	/*
 	 * We don't have the key and we are doing a lookup; decode the
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index e57e8d9..f372fc4 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -771,7 +771,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 		if (err)
 			return ERR_PTR(err);
 		if (!fscrypt_has_encryption_key(dir))
-			return ERR_PTR(-EPERM);
+			return ERR_PTR(-ENOKEY);
 		if (!handle)
 			nblocks += EXT4_DATA_TRANS_BLOCKS(dir->i_sb);
 		encrypt = 1;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index eadba91..80b8afa 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1378,6 +1378,8 @@ static struct buffer_head * ext4_find_entry (struct inode *dir,
 		return NULL;
 
 	retval = ext4_fname_setup_filename(dir, d_name, 1, &fname);
+	if (retval == -ENOENT)
+		return NULL;
 	if (retval)
 		return ERR_PTR(retval);
 
@@ -3088,7 +3090,7 @@ static int ext4_symlink(struct inode *dir,
 		if (err)
 			return err;
 		if (!fscrypt_has_encryption_key(dir))
-			return -EPERM;
+			return -ENOKEY;
 		disk_link.len = (fscrypt_fname_encrypted_size(dir, len) +
 				 sizeof(struct fscrypt_symlink_data));
 		sd = kzalloc(disk_link.len, GFP_KERNEL);
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 369f451..60656b2 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -268,7 +268,10 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 
 	err = fscrypt_setup_filename(dir, child, 1, &fname);
 	if (err) {
-		*res_page = ERR_PTR(err);
+		if (err == -ENOENT)
+			*res_page = NULL;
+		else
+			*res_page = ERR_PTR(err);
 		return NULL;
 	}
 
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 489fa0d..9a2b1f8 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -403,7 +403,7 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
 			return err;
 
 		if (!fscrypt_has_encryption_key(dir))
-			return -EPERM;
+			return -ENOKEY;
 
 		disk_link.len = (fscrypt_fname_encrypted_size(dir, len) +
 				sizeof(struct fscrypt_symlink_data));
@@ -447,7 +447,7 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
 			goto err_out;
 
 		if (!fscrypt_has_encryption_key(inode)) {
-			err = -EPERM;
+			err = -ENOKEY;
 			goto err_out;
 		}
 
-- 
2.8.0.rc3.226.g39d4020

Re: [RFC PATCH 1/5] fscrypt: use ENOKEY when file cannot be created w/o key

[Theodore Ts'o]

On Mon, Dec 05, 2016 at 11:12:44AM -0800, Eric Biggers wrote:
> As part of an effort to clean up fscrypt-related error codes, make
> attempting to create a file in an encrypted directory that hasn't been
> "unlocked" fail with ENOKEY.  Previously, several error codes were used
> for this case, including ENOENT, EACCES, and EPERM, and they were not
> consistent between and within filesystems.  ENOKEY is a better choice
> because it expresses that the failure is due to lacking the encryption
> key.  It also matches the error code returned when trying to open an
> encrypted regular file without the key.
> 
> I am not aware of any users who might be relying on the previous
> inconsistent error codes, which were never documented anywhere.
> 
> This failure case will be exercised by an xfstest.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Applied to the fscrypt branch, thanks.

						- Ted

[RFC PATCH 2/5] fscrypt: use ENOTDIR when setting encryption policy on nondirectory

[Eric Biggers]

As part of an effort to clean up fscrypt-related error codes, make
FS_IOC_SET_ENCRYPTION_POLICY fail with ENOTDIR when the file descriptor
does not refer to a directory.  This is more descriptive than EINVAL,
which was ambiguous with some of the other error cases.

I am not aware of any users who might be relying on the previous error
code of EINVAL, which was never documented anywhere, and in some buggy
kernels did not exist at all as the S_ISDIR() check was missing.

This failure case will be exercised by an xfstest.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index b96a10e..1118f3a 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -116,7 +116,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 
 	if (!inode_has_encryption_context(inode)) {
 		if (!S_ISDIR(inode->i_mode))
-			ret = -EINVAL;
+			ret = -ENOTDIR;
 		else if (!inode->i_sb->s_cop->empty_dir)
 			ret = -EOPNOTSUPP;
 		else if (!inode->i_sb->s_cop->empty_dir(inode))
-- 
2.8.0.rc3.226.g39d4020

Re: [RFC PATCH 2/5] fscrypt: use ENOTDIR when setting encryption policy on nondirectory

[Theodore Ts'o]

On Mon, Dec 05, 2016 at 11:12:45AM -0800, Eric Biggers wrote:
> As part of an effort to clean up fscrypt-related error codes, make
> FS_IOC_SET_ENCRYPTION_POLICY fail with ENOTDIR when the file descriptor
> does not refer to a directory.  This is more descriptive than EINVAL,
> which was ambiguous with some of the other error cases.
> 
> I am not aware of any users who might be relying on the previous error
> code of EINVAL, which was never documented anywhere, and in some buggy
> kernels did not exist at all as the S_ISDIR() check was missing.
> 
> This failure case will be exercised by an xfstest.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Applied to the fscrypt branch, thanks.

				- Ted

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[RFC PATCH 3/5] fscrypt: use EEXIST when file already uses different policy

[Eric Biggers]

As part of an effort to clean up fscrypt-related error codes, make
FS_IOC_SET_ENCRYPTION_POLICY fail with EEXIST when the file already uses
a different encryption policy.  This is more descriptive than EINVAL,
which was ambiguous with some of the other error cases.

I am not aware of any users who might be relying on the previous error
code of EINVAL, which was never documented anywhere.

This failure case will be exercised by an xfstest.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 1118f3a..4811539 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -129,7 +129,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 		printk(KERN_WARNING
 		       "%s: Policy inconsistent with encryption context\n",
 		       __func__);
-		ret = -EINVAL;
+		ret = -EEXIST;
 	}
 
 	inode_unlock(inode);
-- 
2.8.0.rc3.226.g39d4020

Re: [RFC PATCH 3/5] fscrypt: use EEXIST when file already uses different policy

[Theodore Ts'o]

On Mon, Dec 05, 2016 at 11:12:46AM -0800, Eric Biggers wrote:
> As part of an effort to clean up fscrypt-related error codes, make
> FS_IOC_SET_ENCRYPTION_POLICY fail with EEXIST when the file already uses
> a different encryption policy.  This is more descriptive than EINVAL,
> which was ambiguous with some of the other error cases.
> 
> I am not aware of any users who might be relying on the previous error
> code of EINVAL, which was never documented anywhere.
> 
> This failure case will be exercised by an xfstest.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Applied to the fscrypt branch, thanks.

						- Ted

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[RFC PATCH 4/5] fscrypt: remove user-triggerable warning messages

[Eric Biggers]

Several warning messages were not rate limited and were user-triggerable
from FS_IOC_SET_ENCRYPTION_POLICY.  These shouldn't really have been
there in the first place, but either way they aren't as useful now that
the error codes have been improved.  So just remove them.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 4811539..a9f11b3 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -66,20 +66,12 @@ static int create_encryption_context_from_policy(struct inode *inode,
 					FS_KEY_DESCRIPTOR_SIZE);
 
 	if (!fscrypt_valid_contents_enc_mode(
-				policy->contents_encryption_mode)) {
-		printk(KERN_WARNING
-		       "%s: Invalid contents encryption mode %d\n", __func__,
-			policy->contents_encryption_mode);
+				policy->contents_encryption_mode))
 		return -EINVAL;
-	}
 
 	if (!fscrypt_valid_filenames_enc_mode(
-				policy->filenames_encryption_mode)) {
-		printk(KERN_WARNING
-			"%s: Invalid filenames encryption mode %d\n", __func__,
-			policy->filenames_encryption_mode);
+				policy->filenames_encryption_mode))
 		return -EINVAL;
-	}
 
 	if (policy->flags & ~FS_POLICY_FLAGS_VALID)
 		return -EINVAL;
@@ -126,9 +118,6 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 								    &policy);
 	} else if (!is_encryption_context_consistent_with_policy(inode,
 								 &policy)) {
-		printk(KERN_WARNING
-		       "%s: Policy inconsistent with encryption context\n",
-		       __func__);
 		ret = -EEXIST;
 	}
 
-- 
2.8.0.rc3.226.g39d4020

Re: [RFC PATCH 4/5] fscrypt: remove user-triggerable warning messages

[Theodore Ts'o]

On Mon, Dec 05, 2016 at 11:12:47AM -0800, Eric Biggers wrote:
> Several warning messages were not rate limited and were user-triggerable
> from FS_IOC_SET_ENCRYPTION_POLICY.  These shouldn't really have been
> there in the first place, but either way they aren't as useful now that
> the error codes have been improved.  So just remove them.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Applied, thanks.

					- Ted

[RFC PATCH 5/5] fscrypt: pass up error codes from ->get_context()

[Eric Biggers]

It was possible for the ->get_context() operation to fail with a
specific error code, which was then not returned to the caller of
FS_IOC_SET_ENCRYPTION_POLICY or FS_IOC_GET_ENCRYPTION_POLICY.  Make sure
to pass through these error codes.  Also reorganize the code so that
->get_context() only needs to be called one time when setting an
encryption policy, and handle contexts of unrecognized sizes more
appropriately.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 54 +++++++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index a9f11b3..d64ec95 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -13,37 +13,20 @@
 #include <linux/fscrypto.h>
 #include <linux/mount.h>
 
-static int inode_has_encryption_context(struct inode *inode)
-{
-	if (!inode->i_sb->s_cop->get_context)
-		return 0;
-	return (inode->i_sb->s_cop->get_context(inode, NULL, 0L) > 0);
-}
-
 /*
- * check whether the policy is consistent with the encryption context
- * for the inode
+ * check whether an encryption policy is consistent with an encryption context
  */
-static int is_encryption_context_consistent_with_policy(struct inode *inode,
+static bool is_encryption_context_consistent_with_policy(
+				const struct fscrypt_context *ctx,
 				const struct fscrypt_policy *policy)
 {
-	struct fscrypt_context ctx;
-	int res;
-
-	if (!inode->i_sb->s_cop->get_context)
-		return 0;
-
-	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
-	if (res != sizeof(ctx))
-		return 0;
-
-	return (memcmp(ctx.master_key_descriptor, policy->master_key_descriptor,
-			FS_KEY_DESCRIPTOR_SIZE) == 0 &&
-			(ctx.flags == policy->flags) &&
-			(ctx.contents_encryption_mode ==
-			 policy->contents_encryption_mode) &&
-			(ctx.filenames_encryption_mode ==
-			 policy->filenames_encryption_mode));
+	return memcmp(ctx->master_key_descriptor, policy->master_key_descriptor,
+		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+		(ctx->flags == policy->flags) &&
+		(ctx->contents_encryption_mode ==
+		 policy->contents_encryption_mode) &&
+		(ctx->filenames_encryption_mode ==
+		 policy->filenames_encryption_mode);
 }
 
 static int create_encryption_context_from_policy(struct inode *inode,
@@ -90,6 +73,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 	struct fscrypt_policy policy;
 	struct inode *inode = file_inode(filp);
 	int ret;
+	struct fscrypt_context ctx;
 
 	if (copy_from_user(&policy, arg, sizeof(policy)))
 		return -EFAULT;
@@ -106,7 +90,8 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 
 	inode_lock(inode);
 
-	if (!inode_has_encryption_context(inode)) {
+	ret = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
+	if (ret == -ENODATA) {
 		if (!S_ISDIR(inode->i_mode))
 			ret = -ENOTDIR;
 		else if (!inode->i_sb->s_cop->empty_dir)
@@ -116,8 +101,13 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 		else
 			ret = create_encryption_context_from_policy(inode,
 								    &policy);
-	} else if (!is_encryption_context_consistent_with_policy(inode,
-								 &policy)) {
+	} else if (ret == sizeof(ctx) &&
+		   is_encryption_context_consistent_with_policy(&ctx,
+								&policy)) {
+		/* The file already uses the same encryption policy. */
+		ret = 0;
+	} else if (ret >= 0 || ret == -ERANGE) {
+		/* The file already uses a different encryption policy. */
 		ret = -EEXIST;
 	}
 
@@ -140,8 +130,10 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
 		return -ENODATA;
 
 	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
+	if (res < 0 && res != -ERANGE)
+		return res;
 	if (res != sizeof(ctx))
-		return -ENODATA;
+		return -EINVAL;
 	if (ctx.format != FS_ENCRYPTION_CONTEXT_FORMAT_V1)
 		return -EINVAL;
 
-- 
2.8.0.rc3.226.g39d4020

Re: [RFC PATCH 5/5] fscrypt: pass up error codes from ->get_context()

[Theodore Ts'o]

On Mon, Dec 05, 2016 at 11:12:48AM -0800, Eric Biggers wrote:
> It was possible for the ->get_context() operation to fail with a
> specific error code, which was then not returned to the caller of
> FS_IOC_SET_ENCRYPTION_POLICY or FS_IOC_GET_ENCRYPTION_POLICY.  Make sure
> to pass through these error codes.  Also reorganize the code so that
> ->get_context() only needs to be called one time when setting an
> encryption policy, and handle contexts of unrecognized sizes more
> appropriately.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied to the fscrypt branch.

					- Ted

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot