[PATCH 1/3] fscrypt: fix loophole in one-encryption-policy-per-tree enforcement

[PATCH 1/3] fscrypt: fix loophole in one-encryption-policy-per-tree enforcement

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Filesystem encryption is designed to enforce that all files in an
encrypted directory tree use the same encryption policy.  Operations
that violate this constraint are supposed to fail with EPERM.  There was
one case that was missed, however: the cross-rename operation (i.e.
renameat2 with RENAME_EXCHANGE) allowed two files with different
encryption policies to be exchanged, provided that neither encryption
key was available.

To fix this, when we can't compare the fscrypt_info structs because the
key is unavailable, compare the fscrypt_context structs instead.

This will be covered by a test in my encryption xfstests patchset.

Fixes: b7236e21d55f ("ext4 crypto: reorganize how we store keys in the inode")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 76 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 19 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 6ed7c2e..5de0633 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -169,22 +169,46 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
 }
 EXPORT_SYMBOL(fscrypt_ioctl_get_policy);
 
+/**
+ * fscrypt_has_permitted_context() - is a file's encryption policy permitted
+ *				     within its parent directory?
+ *
+ * @parent: inode for parent directory
+ * @child: inode for file being looked up or linked into the directory
+ *
+ * Filesystems must call this function during lookup in an encrypted directory
+ * and before any operation that involves linking a file into an encrypted
+ * directory, including link, rename, and cross rename.  It enforces the
+ * constraint that within a given encrypted directory tree, all files use the
+ * same encryption policy.  The lookup check is needed to detect offline
+ * violations of this constraint, whereas the other checks are needed to prevent
+ * online violations of this constraint.
+ *
+ * Return: 1 if permitted, 0 if forbidden.  If forbidden, the caller must fail
+ * the filesystem operation with EPERM.
+ */
 int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 {
-	struct fscrypt_info *parent_ci, *child_ci;
+	const struct fscrypt_operations *cops = parent->i_sb->s_cop;
+	const struct fscrypt_info *parent_ci, *child_ci;
+	struct fscrypt_context parent_ctx, child_ctx;
 	int res;
 
-	if ((parent == NULL) || (child == NULL)) {
-		printk(KERN_ERR	"parent %p child %p\n", parent, child);
-		BUG_ON(1);
-	}
-
-	/* no restrictions if the parent directory is not encrypted */
-	if (!parent->i_sb->s_cop->is_encrypted(parent))
+	/* No restrictions if the parent directory is unencrypted */
+	if (!cops->is_encrypted(parent))
 		return 1;
-	/* if the child directory is not encrypted, this is always a problem */
-	if (!parent->i_sb->s_cop->is_encrypted(child))
+
+	/* Encrypted directories must not contain unencrypted files */
+	if (!cops->is_encrypted(child))
 		return 0;
+
+	/*
+	 * Both parent and child are encrypted, so verify they use the same
+	 * encryption policy.  Compare the fscrypt_info structs if the keys are
+	 * available, otherwise compare the fscrypt_context structs directly.
+	 * In any case, if an unexpected error occurs, fall back to "forbidden".
+	 */
+
 	res = fscrypt_get_encryption_info(parent);
 	if (res)
 		return 0;
@@ -193,17 +217,31 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 		return 0;
 	parent_ci = parent->i_crypt_info;
 	child_ci = child->i_crypt_info;
-	if (!parent_ci && !child_ci)
-		return 1;
-	if (!parent_ci || !child_ci)
+	if (parent_ci && child_ci) {
+		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
+			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
+			(parent_ci->ci_filename_mode ==
+			 child_ci->ci_filename_mode) &&
+			(parent_ci->ci_flags == child_ci->ci_flags);
+	}
+
+	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
+	if (res != sizeof(parent_ctx))
 		return 0;
 
-	return (memcmp(parent_ci->ci_master_key,
-			child_ci->ci_master_key,
-			FS_KEY_DESCRIPTOR_SIZE) == 0 &&
-		(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
-		(parent_ci->ci_filename_mode == child_ci->ci_filename_mode) &&
-		(parent_ci->ci_flags == child_ci->ci_flags));
+	res = cops->get_context(child, &child_ctx, sizeof(child_ctx));
+	if (res != sizeof(child_ctx))
+		return 0;
+
+	return memcmp(parent_ctx.master_key_descriptor,
+		      child_ctx.master_key_descriptor,
+		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+		(parent_ctx.contents_encryption_mode ==
+		 child_ctx.contents_encryption_mode) &&
+		(parent_ctx.filenames_encryption_mode ==
+		 child_ctx.filenames_encryption_mode) &&
+		(parent_ctx.flags == child_ctx.flags);
 }
 EXPORT_SYMBOL(fscrypt_has_permitted_context);
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/3] fscrypt: fix loophole in one-encryption-policy-per-tree enforcement

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Filesystem encryption is designed to enforce that all files in an
encrypted directory tree use the same encryption policy.  Operations
that violate this constraint are supposed to fail with EPERM.  There was
one case that was missed, however: the cross-rename operation (i.e.
renameat2 with RENAME_EXCHANGE) allowed two files with different
encryption policies to be exchanged, provided that neither encryption
key was available.

To fix this, when we can't compare the fscrypt_info structs because the
key is unavailable, compare the fscrypt_context structs instead.

This will be covered by a test in my encryption xfstests patchset.

Fixes: b7236e21d55f ("ext4 crypto: reorganize how we store keys in the inode")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 76 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 19 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 6ed7c2e..5de0633 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -169,22 +169,46 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
 }
 EXPORT_SYMBOL(fscrypt_ioctl_get_policy);
 
+/**
+ * fscrypt_has_permitted_context() - is a file's encryption policy permitted
+ *				     within its parent directory?
+ *
+ * @parent: inode for parent directory
+ * @child: inode for file being looked up or linked into the directory
+ *
+ * Filesystems must call this function during lookup in an encrypted directory
+ * and before any operation that involves linking a file into an encrypted
+ * directory, including link, rename, and cross rename.  It enforces the
+ * constraint that within a given encrypted directory tree, all files use the
+ * same encryption policy.  The lookup check is needed to detect offline
+ * violations of this constraint, whereas the other checks are needed to prevent
+ * online violations of this constraint.
+ *
+ * Return: 1 if permitted, 0 if forbidden.  If forbidden, the caller must fail
+ * the filesystem operation with EPERM.
+ */
 int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 {
-	struct fscrypt_info *parent_ci, *child_ci;
+	const struct fscrypt_operations *cops = parent->i_sb->s_cop;
+	const struct fscrypt_info *parent_ci, *child_ci;
+	struct fscrypt_context parent_ctx, child_ctx;
 	int res;
 
-	if ((parent == NULL) || (child == NULL)) {
-		printk(KERN_ERR	"parent %p child %p\n", parent, child);
-		BUG_ON(1);
-	}
-
-	/* no restrictions if the parent directory is not encrypted */
-	if (!parent->i_sb->s_cop->is_encrypted(parent))
+	/* No restrictions if the parent directory is unencrypted */
+	if (!cops->is_encrypted(parent))
 		return 1;
-	/* if the child directory is not encrypted, this is always a problem */
-	if (!parent->i_sb->s_cop->is_encrypted(child))
+
+	/* Encrypted directories must not contain unencrypted files */
+	if (!cops->is_encrypted(child))
 		return 0;
+
+	/*
+	 * Both parent and child are encrypted, so verify they use the same
+	 * encryption policy.  Compare the fscrypt_info structs if the keys are
+	 * available, otherwise compare the fscrypt_context structs directly.
+	 * In any case, if an unexpected error occurs, fall back to "forbidden".
+	 */
+
 	res = fscrypt_get_encryption_info(parent);
 	if (res)
 		return 0;
@@ -193,17 +217,31 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 		return 0;
 	parent_ci = parent->i_crypt_info;
 	child_ci = child->i_crypt_info;
-	if (!parent_ci && !child_ci)
-		return 1;
-	if (!parent_ci || !child_ci)
+	if (parent_ci && child_ci) {
+		return memcmp(parent_ci->ci_master_key, child_ci->ci_master_key,
+			      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+			(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
+			(parent_ci->ci_filename_mode ==
+			 child_ci->ci_filename_mode) &&
+			(parent_ci->ci_flags == child_ci->ci_flags);
+	}
+
+	res = cops->get_context(parent, &parent_ctx, sizeof(parent_ctx));
+	if (res != sizeof(parent_ctx))
 		return 0;
 
-	return (memcmp(parent_ci->ci_master_key,
-			child_ci->ci_master_key,
-			FS_KEY_DESCRIPTOR_SIZE) == 0 &&
-		(parent_ci->ci_data_mode == child_ci->ci_data_mode) &&
-		(parent_ci->ci_filename_mode == child_ci->ci_filename_mode) &&
-		(parent_ci->ci_flags == child_ci->ci_flags));
+	res = cops->get_context(child, &child_ctx, sizeof(child_ctx));
+	if (res != sizeof(child_ctx))
+		return 0;
+
+	return memcmp(parent_ctx.master_key_descriptor,
+		      child_ctx.master_key_descriptor,
+		      FS_KEY_DESCRIPTOR_SIZE) == 0 &&
+		(parent_ctx.contents_encryption_mode ==
+		 child_ctx.contents_encryption_mode) &&
+		(parent_ctx.filenames_encryption_mode ==
+		 child_ctx.filenames_encryption_mode) &&
+		(parent_ctx.flags == child_ctx.flags);
 }
 EXPORT_SYMBOL(fscrypt_has_permitted_context);
 
-- 
2.8.0.rc3.226.g39d4020


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

[PATCH 2/3] fscrypt: fix renaming and linking special files

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Attempting to link a device node, named pipe, or socket file into an
encrypted directory through rename(2) or link(2) always failed with
EPERM.  This happened because fscrypt_permitted_context() saw that the
file was unencrypted and forbid creating the link.  This behavior was
unexpected because such files are never encrypted; only regular files,
directories, and symlinks can be encrypted.

To fix this, make fscrypt_has_permitted_context() always return true on
special files.

This will be covered by a test in my encryption xfstests patchset.

Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt support")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 5de0633..2e50cbc 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -198,6 +198,11 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 	if (!cops->is_encrypted(parent))
 		return 1;
 
+	/* No restrictions on file types which are never encrypted */
+	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
+	    !S_ISLNK(child->i_mode))
+		return 1;
+
 	/* Encrypted directories must not contain unencrypted files */
 	if (!cops->is_encrypted(child))
 		return 0;
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/3] fscrypt: fix renaming and linking special files

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Attempting to link a device node, named pipe, or socket file into an
encrypted directory through rename(2) or link(2) always failed with
EPERM.  This happened because fscrypt_permitted_context() saw that the
file was unencrypted and forbid creating the link.  This behavior was
unexpected because such files are never encrypted; only regular files,
directories, and symlinks can be encrypted.

To fix this, make fscrypt_has_permitted_context() always return true on
special files.

This will be covered by a test in my encryption xfstests patchset.

Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt support")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 5de0633..2e50cbc 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -198,6 +198,11 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 	if (!cops->is_encrypted(parent))
 		return 1;
 
+	/* No restrictions on file types which are never encrypted */
+	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
+	    !S_ISLNK(child->i_mode))
+		return 1;
+
 	/* Encrypted directories must not contain unencrypted files */
 	if (!cops->is_encrypted(child))
 		return 0;
-- 
2.8.0.rc3.226.g39d4020


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

[PATCH 3/3] fscrypt: consolidate fscrypt_has_permitted_context() checks

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Now that fscrypt_has_permitted_context() compares the fscrypt_context
rather than the fscrypt_info when needed, it is no longer necessary to
delay fscrypt_has_permitted_context() from ->lookup() to ->open() for
regular files, as introduced in commit ff978b09f973 ("ext4 crypto: move
context consistency check to ext4_file_open()").  Therefore the check in
->open(), along with the dget_parent() hack, can be removed.

It's also no longer necessary to check the file type before calling
fscrypt_has_permitted_context().

This patch makes these changes for both ext4 and f2fs.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/file.c  | 12 ------------
 fs/ext4/namei.c | 10 ++--------
 fs/f2fs/file.c  | 15 +++++----------
 fs/f2fs/namei.c |  7 ++-----
 4 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index b5f1844..2123cd8 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -398,7 +398,6 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 	struct super_block *sb = inode->i_sb;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct vfsmount *mnt = filp->f_path.mnt;
-	struct dentry *dir;
 	struct path path;
 	char buf[64], *cp;
 	int ret;
@@ -443,17 +442,6 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 			return -ENOKEY;
 	}
 
-	dir = dget_parent(file_dentry(filp));
-	if (ext4_encrypted_inode(d_inode(dir)) &&
-			!fscrypt_has_permitted_context(d_inode(dir), inode)) {
-		ext4_warning(inode->i_sb,
-			     "Inconsistent encryption contexts: %lu/%lu",
-			     (unsigned long) d_inode(dir)->i_ino,
-			     (unsigned long) inode->i_ino);
-		dput(dir);
-		return -EPERM;
-	}
-	dput(dir);
 	/*
 	 * Set up the jbd2_inode if we are opening the inode for
 	 * writing and the journal is present
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index eadba91..eb8b064 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1612,17 +1612,11 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 			return ERR_PTR(-EFSCORRUPTED);
 		}
 		if (!IS_ERR(inode) && ext4_encrypted_inode(dir) &&
-		    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
 		    !fscrypt_has_permitted_context(dir, inode)) {
-			int nokey = ext4_encrypted_inode(inode) &&
-				!fscrypt_has_encryption_key(inode);
-			iput(inode);
-			if (nokey)
-				return ERR_PTR(-ENOKEY);
 			ext4_warning(inode->i_sb,
 				     "Inconsistent encryption contexts: %lu/%lu",
-				     (unsigned long) dir->i_ino,
-				     (unsigned long) inode->i_ino);
+				     dir->i_ino, inode->i_ino);
+			iput(inode);
 			return ERR_PTR(-EPERM);
 		}
 	}
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 49f10dc..381d39b 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -443,23 +443,18 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma)
 static int f2fs_file_open(struct inode *inode, struct file *filp)
 {
 	int ret = generic_file_open(inode, filp);
-	struct dentry *dir;
 
-	if (!ret && f2fs_encrypted_inode(inode)) {
+	if (ret)
+		return ret;
+
+	if (f2fs_encrypted_inode(inode)) {
 		ret = fscrypt_get_encryption_info(inode);
 		if (ret)
 			return -EACCES;
 		if (!fscrypt_has_encryption_key(inode))
 			return -ENOKEY;
 	}
-	dir = dget_parent(file_dentry(filp));
-	if (f2fs_encrypted_inode(d_inode(dir)) &&
-			!fscrypt_has_permitted_context(d_inode(dir), inode)) {
-		dput(dir);
-		return -EPERM;
-	}
-	dput(dir);
-	return ret;
+	return 0;
 }
 
 int truncate_data_blocks_range(struct dnode_of_data *dn, int count)
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index db33b56..980f783 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -322,11 +322,8 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 			goto err_out;
 	}
 	if (!IS_ERR(inode) && f2fs_encrypted_inode(dir) &&
-			(S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
-			!fscrypt_has_permitted_context(dir, inode)) {
-		bool nokey = f2fs_encrypted_inode(inode) &&
-			!fscrypt_has_encryption_key(inode);
-		err = nokey ? -ENOKEY : -EPERM;
+	    !fscrypt_has_permitted_context(dir, inode)) {
+		err = -EPERM;
 		goto err_out;
 	}
 	return d_splice_alias(inode, dentry);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/3] fscrypt: consolidate fscrypt_has_permitted_context() checks

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Now that fscrypt_has_permitted_context() compares the fscrypt_context
rather than the fscrypt_info when needed, it is no longer necessary to
delay fscrypt_has_permitted_context() from ->lookup() to ->open() for
regular files, as introduced in commit ff978b09f973 ("ext4 crypto: move
context consistency check to ext4_file_open()").  Therefore the check in
->open(), along with the dget_parent() hack, can be removed.

It's also no longer necessary to check the file type before calling
fscrypt_has_permitted_context().

This patch makes these changes for both ext4 and f2fs.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/file.c  | 12 ------------
 fs/ext4/namei.c | 10 ++--------
 fs/f2fs/file.c  | 15 +++++----------
 fs/f2fs/namei.c |  7 ++-----
 4 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index b5f1844..2123cd8 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -398,7 +398,6 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 	struct super_block *sb = inode->i_sb;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct vfsmount *mnt = filp->f_path.mnt;
-	struct dentry *dir;
 	struct path path;
 	char buf[64], *cp;
 	int ret;
@@ -443,17 +442,6 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 			return -ENOKEY;
 	}
 
-	dir = dget_parent(file_dentry(filp));
-	if (ext4_encrypted_inode(d_inode(dir)) &&
-			!fscrypt_has_permitted_context(d_inode(dir), inode)) {
-		ext4_warning(inode->i_sb,
-			     "Inconsistent encryption contexts: %lu/%lu",
-			     (unsigned long) d_inode(dir)->i_ino,
-			     (unsigned long) inode->i_ino);
-		dput(dir);
-		return -EPERM;
-	}
-	dput(dir);
 	/*
 	 * Set up the jbd2_inode if we are opening the inode for
 	 * writing and the journal is present
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index eadba91..eb8b064 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1612,17 +1612,11 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 			return ERR_PTR(-EFSCORRUPTED);
 		}
 		if (!IS_ERR(inode) && ext4_encrypted_inode(dir) &&
-		    (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
 		    !fscrypt_has_permitted_context(dir, inode)) {
-			int nokey = ext4_encrypted_inode(inode) &&
-				!fscrypt_has_encryption_key(inode);
-			iput(inode);
-			if (nokey)
-				return ERR_PTR(-ENOKEY);
 			ext4_warning(inode->i_sb,
 				     "Inconsistent encryption contexts: %lu/%lu",
-				     (unsigned long) dir->i_ino,
-				     (unsigned long) inode->i_ino);
+				     dir->i_ino, inode->i_ino);
+			iput(inode);
 			return ERR_PTR(-EPERM);
 		}
 	}
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 49f10dc..381d39b 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -443,23 +443,18 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma)
 static int f2fs_file_open(struct inode *inode, struct file *filp)
 {
 	int ret = generic_file_open(inode, filp);
-	struct dentry *dir;
 
-	if (!ret && f2fs_encrypted_inode(inode)) {
+	if (ret)
+		return ret;
+
+	if (f2fs_encrypted_inode(inode)) {
 		ret = fscrypt_get_encryption_info(inode);
 		if (ret)
 			return -EACCES;
 		if (!fscrypt_has_encryption_key(inode))
 			return -ENOKEY;
 	}
-	dir = dget_parent(file_dentry(filp));
-	if (f2fs_encrypted_inode(d_inode(dir)) &&
-			!fscrypt_has_permitted_context(d_inode(dir), inode)) {
-		dput(dir);
-		return -EPERM;
-	}
-	dput(dir);
-	return ret;
+	return 0;
 }
 
 int truncate_data_blocks_range(struct dnode_of_data *dn, int count)
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index db33b56..980f783 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -322,11 +322,8 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 			goto err_out;
 	}
 	if (!IS_ERR(inode) && f2fs_encrypted_inode(dir) &&
-			(S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) &&
-			!fscrypt_has_permitted_context(dir, inode)) {
-		bool nokey = f2fs_encrypted_inode(inode) &&
-			!fscrypt_has_encryption_key(inode);
-		err = nokey ? -ENOKEY : -EPERM;
+	    !fscrypt_has_permitted_context(dir, inode)) {
+		err = -EPERM;
 		goto err_out;
 	}
 	return d_splice_alias(inode, dentry);
-- 
2.8.0.rc3.226.g39d4020


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

Re: [PATCH 1/3] fscrypt: fix loophole in one-encryption-policy-per-tree enforcement

[Richard Weinberger]

On 15.12.2016 20:19, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Filesystem encryption is designed to enforce that all files in an
> encrypted directory tree use the same encryption policy.  Operations
> that violate this constraint are supposed to fail with EPERM.  There was
> one case that was missed, however: the cross-rename operation (i.e.
> renameat2 with RENAME_EXCHANGE) allowed two files with different
> encryption policies to be exchanged, provided that neither encryption
> key was available.
> 
> To fix this, when we can't compare the fscrypt_info structs because the
> key is unavailable, compare the fscrypt_context structs instead.
> 
> This will be covered by a test in my encryption xfstests patchset.
> 
> Fixes: b7236e21d55f ("ext4 crypto: reorganize how we store keys in the inode")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 3/3] fscrypt: consolidate fscrypt_has_permitted_context() checks

[Richard Weinberger]

On 15.12.2016 20:19, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Now that fscrypt_has_permitted_context() compares the fscrypt_context
> rather than the fscrypt_info when needed, it is no longer necessary to
> delay fscrypt_has_permitted_context() from ->lookup() to ->open() for
> regular files, as introduced in commit ff978b09f973 ("ext4 crypto: move
> context consistency check to ext4_file_open()").  Therefore the check in
> ->open(), along with the dget_parent() hack, can be removed.
> 
> It's also no longer necessary to check the file type before calling
> fscrypt_has_permitted_context().
> 
> This patch makes these changes for both ext4 and f2fs.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/ext4/file.c  | 12 ------------
>  fs/ext4/namei.c | 10 ++--------
>  fs/f2fs/file.c  | 15 +++++----------
>  fs/f2fs/namei.c |  7 ++-----
>  4 files changed, 9 insertions(+), 35 deletions(-)

Can please also take care of UBIFS? :-)

Thanks,
//richard

Re: [PATCH 2/3] fscrypt: fix renaming and linking special files

[Richard Weinberger]

On 15.12.2016 20:19, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Attempting to link a device node, named pipe, or socket file into an
> encrypted directory through rename(2) or link(2) always failed with
> EPERM.  This happened because fscrypt_permitted_context() saw that the
> file was unencrypted and forbid creating the link.  This behavior was
> unexpected because such files are never encrypted; only regular files,
> directories, and symlinks can be encrypted.
> 
> To fix this, make fscrypt_has_permitted_context() always return true on
> special files.
> 
> This will be covered by a test in my encryption xfstests patchset.
> 
> Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt support")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 3/3] fscrypt: consolidate fscrypt_has_permitted_context() checks

[Eric Biggers]

On Fri, Dec 16, 2016 at 01:22:51PM +0100, Richard Weinberger wrote:
> On 15.12.2016 20:19, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Now that fscrypt_has_permitted_context() compares the fscrypt_context
> > rather than the fscrypt_info when needed, it is no longer necessary to
> > delay fscrypt_has_permitted_context() from ->lookup() to ->open() for
> > regular files, as introduced in commit ff978b09f973 ("ext4 crypto: move
> > context consistency check to ext4_file_open()").  Therefore the check in
> > ->open(), along with the dget_parent() hack, can be removed.
> > 
> > It's also no longer necessary to check the file type before calling
> > fscrypt_has_permitted_context().
> > 
> > This patch makes these changes for both ext4 and f2fs.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  fs/ext4/file.c  | 12 ------------
> >  fs/ext4/namei.c | 10 ++--------
> >  fs/f2fs/file.c  | 15 +++++----------
> >  fs/f2fs/namei.c |  7 ++-----
> >  4 files changed, 9 insertions(+), 35 deletions(-)
> 
> Can please also take care of UBIFS? :-)
> 
> Thanks,
> //richard

Yes, I see that UBIFS encryption just got merged yesterday, so I'll send a
version that updates UBIFS too.  And it seems the
fscrypt_has_permitted_context() call in ubifs_lookup() is missing, so I'll add
that too.

I'm wondering if it would make more sense to do a separate patch for each
filesystem?  But in this case the filesystem changes are dependent on the prior
patches to fs/crypto/, so they can't simply be sent through the per-filesystem
trees unless each one merges in the fs/crypto/ changes too.

Eric