Skip to contentSkip to conversation

fs-verity: fix !CONFIG_FS_VERITY case

PATCH In review Eric Biggers 3 participants 7 messages Dec 11, 2018 – Dec 12, 2018

Subsystems (4)
ext4 file system F2FS file system fscrypt: file system level encryption support fs-verity: read-only file-based authenticity protection
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

cover fs-verity: fix !CONFIG_FS_VERITY case

Replace the two patches that broken the !CONFIG_FS_VERITY case with fixed verisons.

Chandan Rajendra (2):
fsverity: Move verity status check to fsverity_file_open
fsverity: Move verity status check to fsverity_prepare_setattr

 fs/ext4/file.c           |  8 +++----
 fs/ext4/inode.c          |  8 +++----
 fs/f2fs/file.c           | 16 +++++--------
 fs/verity/setup.c        | 32 ++++---------------------
 include/linux/fsverity.h | 50 ++++++++++++++++++++++++++++++++++++----
 5 files changed, 61 insertions(+), 53 deletions(-)
Signature · 1 line
2.20.0.405.gbc1bbc6f85-goog
author maintainer
#1 lore raw

PATCH 1/2 fsverity: Move verity status check to fsverity_file_open

From: Chandan Rajendra <[email protected]>

Instead of conditionally checking for verity status of an inode before invoking fsverity_file_open(), this commit moves the check inside the definition of fsverity_file_open().

Signed-off-by Chandan Rajendra <[email protected]>

(EB: fix the !CONFIG_FS_VERITY case and inline the IS_VERITY() check)

Signed-off-by Eric Biggers <[email protected]>
---
 fs/ext4/file.c           |  8 +++-----
 fs/f2fs/file.c           |  8 +++-----
 fs/verity/setup.c        | 18 ++----------------
 include/linux/fsverity.h | 25 +++++++++++++++++++++++--
 4 files changed, 31 insertions(+), 28 deletions(-)
fs/ext4/file.c +3 -5
index 30fbd663354f9..b404a857cd487 100644
@@ -444,11 +444,9 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 	if (ret)
 		return ret;
 
-	if (IS_VERITY(inode)) {
-		ret = fsverity_file_open(inode, filp);
-		if (ret)
-			return ret;
-	}
+	ret = fsverity_file_open(inode, filp);
+	if (ret)
+		return ret;
 
 	/*
 	 * Set up the jbd2_inode if we are opening the inode for
fs/f2fs/file.c +3 -5
index c2746f10e119f..6eea508d8656e 100644
@@ -491,11 +491,9 @@ static int f2fs_file_open(struct inode *inode, struct file *filp)
 	if (err)
 		return err;
 
-	if (IS_VERITY(inode)) {
-		err = fsverity_file_open(inode, filp);
-		if (err)
-			return err;
-	}
+	err = fsverity_file_open(inode, filp);
+	if (err)
+		return err;
 
 	filp->f_mode |= FMODE_NOWAIT;
fs/verity/setup.c +2 -16
index 08b609127531b..4ecaeb89166b4 100644
@@ -755,21 +755,7 @@ static int setup_fsverity_info(struct inode *inode)
 	return 0;
 }
 
-/**
- * fsverity_file_open - prepare to open a verity file
- * @inode: the inode being opened
- * @filp: the struct file being set up
- *
- * When opening a verity file, deny the open if it is for writing.  Otherwise,
- * set up the inode's ->i_verity_info (if not already done) by parsing the
- * verity metadata at the end of the file.
- *
- * When combined with fscrypt, this must be called after fscrypt_file_open().
- * Otherwise, we won't have the key set up to decrypt the verity metadata.
- *
- * Return: 0 on success, -errno on failure
- */
-int fsverity_file_open(struct inode *inode, struct file *filp)
+int __fsverity_file_open(struct inode *inode, struct file *filp)
 {
 	if (filp->f_mode & FMODE_WRITE) {
 		pr_debug("Denying opening verity file (ino %lu) for write\n",

@@ -779,7 +765,7 @@ int fsverity_file_open(struct inode *inode, struct file *filp)
 
 	return setup_fsverity_info(inode);
 }
-EXPORT_SYMBOL_GPL(fsverity_file_open);
+EXPORT_SYMBOL_GPL(__fsverity_file_open);
 
 /**
  * fsverity_prepare_setattr - prepare to change a verity inode's attributes
include/linux/fsverity.h +23 -2
index ea8c418bd7d53..0ce170c2c1676 100644
@@ -25,7 +25,7 @@ extern int fsverity_ioctl_enable(struct file *filp, const void __user *arg);
 extern int fsverity_ioctl_measure(struct file *filp, void __user *arg);
 
 /* setup.c */
-extern int fsverity_file_open(struct inode *inode, struct file *filp);
+extern int __fsverity_file_open(struct inode *inode, struct file *filp);
 extern int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
 extern int fsverity_prepare_getattr(struct inode *inode);
 extern void fsverity_cleanup_inode(struct inode *inode);

@@ -58,7 +58,7 @@ static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
 
 /* setup.c */
 
-static inline int fsverity_file_open(struct inode *inode, struct file *filp)
+static inline int __fsverity_file_open(struct inode *inode, struct file *filp)
 {
 	return -EOPNOTSUPP;
 }

@@ -108,4 +108,25 @@ static inline bool fsverity_check_hole(struct inode *inode, struct page *page)
 
 #endif	/* ! CONFIG_FS_VERITY */
 
+/**
+ * fsverity_file_open - prepare to open a verity file
+ * @inode: the inode being opened
+ * @filp: the struct file being set up
+ *
+ * When opening a verity file, deny the open if it is for writing.  Otherwise,
+ * set up the inode's ->i_verity_info (if not already done) by parsing the
+ * verity metadata at the end of the file.
+ *
+ * When combined with fscrypt, this must be called after fscrypt_file_open().
+ * Otherwise, we won't have the key set up to decrypt the verity metadata.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static inline int fsverity_file_open(struct inode *inode, struct file *filp)
+{
+	if (IS_VERITY(inode))
+		return __fsverity_file_open(inode, filp);
+	return 0;
+}
+
 #endif	/* _LINUX_FSVERITY_H */
Signature · 1 line
2.20.0.405.gbc1bbc6f85-goog
author maintainer
#2 lore raw

PATCH 2/2 fsverity: Move verity status check to fsverity_prepare_setattr

In reply to Eric Biggers

From: Chandan Rajendra <[email protected]>

Instead of conditionally checking for verity status of an inode before invoking fsverity_prepare_setattr(), this commit moves the check inside the definition of fsverity_prepare_setattr().

Signed-off-by Chandan Rajendra <[email protected]>

(EB: fix the !CONFIG_FS_VERITY case and inline the IS_VERITY() check)

Signed-off-by Eric Biggers <[email protected]>
---
 fs/ext4/inode.c          |  8 +++-----
 fs/f2fs/file.c           |  8 +++-----
 fs/verity/setup.c        | 14 ++------------
 include/linux/fsverity.h | 25 ++++++++++++++++++++++---
 4 files changed, 30 insertions(+), 25 deletions(-)
fs/ext4/inode.c +3 -5
index b83ab0e812483..817b67c3083be 100644
@@ -5510,11 +5510,9 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
-	if (IS_VERITY(inode)) {
-		error = fsverity_prepare_setattr(dentry, attr);
-		if (error)
-			return error;
-	}
+	error = fsverity_prepare_setattr(dentry, attr);
+	if (error)
+		return error;
 
 	if (is_quota_modification(inode, attr)) {
 		error = dquot_initialize(inode);
fs/f2fs/file.c +3 -5
index 6eea508d8656e..b73609a43baa7 100644
@@ -798,11 +798,9 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 	if (err)
 		return err;
 
-	if (IS_VERITY(inode)) {
-		err = fsverity_prepare_setattr(dentry, attr);
-		if (err)
-			return err;
-	}
+	err = fsverity_prepare_setattr(dentry, attr);
+	if (err)
+		return err;
 
 	if (is_quota_modification(inode, attr)) {
 		err = dquot_initialize(inode);
fs/verity/setup.c +2 -12
index 4ecaeb89166b4..2b707589999c4 100644
@@ -767,17 +767,7 @@ int __fsverity_file_open(struct inode *inode, struct file *filp)
 }
 EXPORT_SYMBOL_GPL(__fsverity_file_open);
 
-/**
- * fsverity_prepare_setattr - prepare to change a verity inode's attributes
- * @dentry: dentry through which the inode is being changed
- * @attr: attributes to change
- *
- * Verity files are immutable, so deny truncates.  This isn't covered by the
- * open-time check because sys_truncate() takes a path, not a file descriptor.
- *
- * Return: 0 on success, -errno on failure
- */
-int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr)
+int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	if (attr->ia_valid & ATTR_SIZE) {
 		pr_debug("Denying truncate of verity file (ino %lu)\n",

@@ -786,7 +776,7 @@ int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(fsverity_prepare_setattr);
+EXPORT_SYMBOL_GPL(__fsverity_prepare_setattr);
 
 /**
  * fsverity_prepare_getattr - prepare to get a verity inode's attributes
include/linux/fsverity.h +22 -3
index 0ce170c2c1676..099fc34936fa6 100644
@@ -26,7 +26,8 @@ extern int fsverity_ioctl_measure(struct file *filp, void __user *arg);
 
 /* setup.c */
 extern int __fsverity_file_open(struct inode *inode, struct file *filp);
-extern int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
+extern int __fsverity_prepare_setattr(struct dentry *dentry,
+				      struct iattr *attr);
 extern int fsverity_prepare_getattr(struct inode *inode);
 extern void fsverity_cleanup_inode(struct inode *inode);
 extern loff_t fsverity_full_i_size(const struct inode *inode);

@@ -63,8 +64,8 @@ static inline int __fsverity_file_open(struct inode *inode, struct file *filp)
 	return -EOPNOTSUPP;
 }
 
-static inline int fsverity_prepare_setattr(struct dentry *dentry,
-					   struct iattr *attr)
+static inline int __fsverity_prepare_setattr(struct dentry *dentry,
+					     struct iattr *attr)
 {
 	return -EOPNOTSUPP;
 }

@@ -129,4 +130,22 @@ static inline int fsverity_file_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/**
+ * fsverity_prepare_setattr - prepare to change a verity inode's attributes
+ * @dentry: dentry through which the inode is being changed
+ * @attr: attributes to change
+ *
+ * Verity files are immutable, so deny truncates.  This isn't covered by the
+ * open-time check because sys_truncate() takes a path, not a file descriptor.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static inline int fsverity_prepare_setattr(struct dentry *dentry,
+					   struct iattr *attr)
+{
+	if (IS_VERITY(d_inode(dentry)))
+		return __fsverity_prepare_setattr(dentry, attr);
+	return 0;
+}
+
 #endif	/* _LINUX_FSVERITY_H */
Signature · 1 line
2.20.0.405.gbc1bbc6f85-goog
maintainer
#4 lore raw
In reply to Eric Biggers
Quoted 2 lines · Eric Biggers

Replace the two patches that broken the !CONFIG_FS_VERITY case with fixed verisons.

I had fixed this by simply adding the conditional to the !CONFIG_FS_VERITY versions of fsverity_file_open() and fsverity_prepare_setattr(), before I found your patch in my inbox. I think my version is simpler (and results in a fewer lines of code :-), so I think I'm going to stick with it.

The net diff of my changes from the previous version was:

include/linux/fsverity.h +2 -2
index ea8c418bd7d5..6684bb72bbfc 100644
@@ -60,13 +60,13 @@ static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
 
 static inline int fsverity_file_open(struct inode *inode, struct file *filp)
 {
-	return -EOPNOTSUPP;
+	return IS_VERITY(inode) ? -ENOTSUPP : 0;
 }
 
 static inline int fsverity_prepare_setattr(struct dentry *dentry,
 					   struct iattr *attr)
 {
-	return -EOPNOTSUPP;
+	return IS_VERITY(d_inode(dentry)) ? -ENOTSUPP : 0;
 }
 
 static inline int fsverity_prepare_getattr(struct inode *inode)

- Ted

author maintainer
#5 lore raw

Hi Ted,

Quoted 33 lines · Theodore Y. Ts'o
Quoted 2 lines · Eric Biggers

Replace the two patches that broken the !CONFIG_FS_VERITY case with fixed verisons.

I had fixed this by simply adding the conditional to the !CONFIG_FS_VERITY versions of fsverity_file_open() and fsverity_prepare_setattr(), before I found your patch in my inbox. I think my version is simpler (and results in a fewer lines of code :-), so I think I'm going to stick with it.

The net diff of my changes from the previous version was:

include/linux/fsverity.h +2 -2
index ea8c418bd7d5..6684bb72bbfc 100644
@@ -60,13 +60,13 @@ static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
 
 static inline int fsverity_file_open(struct inode *inode, struct file *filp)
 {
-	return -EOPNOTSUPP;
+	return IS_VERITY(inode) ? -ENOTSUPP : 0;
 }
 
 static inline int fsverity_prepare_setattr(struct dentry *dentry,
 					   struct iattr *attr)
 {
-	return -EOPNOTSUPP;
+	return IS_VERITY(d_inode(dentry)) ? -ENOTSUPP : 0;
 }
 
 static inline int fsverity_prepare_getattr(struct inode *inode)
Quoted 1 line

- Ted

Either works, but I slightly prefer my version since it minimizes the overhead on non-verity files when the kconfig option is enabled: it's just an i_flags check, rather than a function call plus an i_flags check. The same approach is used in the fscrypt hooks. Also shouldn't it be EOPNOTSUPP, not ENOTSUPP?

- Eric

maintainer
#6 lore raw
Quoted 5 lines · Eric Biggers

Either works, but I slightly prefer my version since it minimizes the overhead on non-verity files when the kconfig option is enabled: it's just an i_flags check, rather than a function call plus an i_flags check. The same approach is used in the fscrypt hooks. Also shouldn't it be EOPNOTSUPP, not ENOTSUPP?

It's the same for both, since in the !CONFIG_FS_VERITY case the two functions are inline functions.

There's actually a bigger issue that I've been meaning to raise, which is that right now we'll set S_VERITY even if the VERITY feature flag is not set. What we should do, in my opinion, is to make ext4 fail a r/w mount if it tries to mount the file system with the VERITY feature flag set and the kernel is mounted with the VERITY flag set.

Also, if the kernel is compiled without CONFIG_FS_VERITY enabled, IS_VERITY(inode) should be defined to 0, and if the an inode is found with EXT4_VERITY_FL and the VERITY feature is not set, we should declare the file system corrupted.

The problem is that f2fs doesn't check **any** file system features at all. If you mount a file system with f2fs features that the kernel doesn't support, the f2fs file system doesn't say boo. I suspect hilarity would ensue if new f2fs file system with new features are mounted on an older kernel, or a kernel where features like encryption and verity are disabled.

So I could easily make these changes for ext4, but what we would do for f2fs isn't clear. I think f2fs is very broken here, but I hesitate defining IS_VERITY(x) to 0 if !CONFIG_FS_VERITY might cause f2fs to break even more than it does today. I would prefer to do that since it would remove dead code from ext4 and f2fs in the !CONFIG_FS_VERITY case, but I think we clarify with the f2fs folks why it is that they aren't checking the f2fs feature mask when mounting a file system first.

- Ted