[PATCH 0/2] fs-verity: fix !CONFIG_FS_VERITY case

[PATCH 0/2] fs-verity: fix !CONFIG_FS_VERITY case

[Eric Biggers]

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(-)

-- 
2.20.0.405.gbc1bbc6f85-goog

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

[Eric Biggers]

From: Chandan Rajendra <chandan@linux.vnet.ibm.com>

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 <chandan@linux.vnet.ibm.com>
(EB: fix the !CONFIG_FS_VERITY case and inline the IS_VERITY() check)
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 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(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 30fbd663354f9..b404a857cd487 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -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
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index c2746f10e119f..6eea508d8656e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -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;
 
diff --git a/fs/verity/setup.c b/fs/verity/setup.c
index 08b609127531b..4ecaeb89166b4 100644
--- a/fs/verity/setup.c
+++ b/fs/verity/setup.c
@@ -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
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index ea8c418bd7d53..0ce170c2c1676 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -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 */
-- 
2.20.0.405.gbc1bbc6f85-goog

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

[Eric Biggers]

From: Chandan Rajendra <chandan@linux.vnet.ibm.com>

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 <chandan@linux.vnet.ibm.com>
(EB: fix the !CONFIG_FS_VERITY case and inline the IS_VERITY() check)
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 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(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b83ab0e812483..817b67c3083be 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 6eea508d8656e..b73609a43baa7 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -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);
diff --git a/fs/verity/setup.c b/fs/verity/setup.c
index 4ecaeb89166b4..2b707589999c4 100644
--- a/fs/verity/setup.c
+++ b/fs/verity/setup.c
@@ -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
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 0ce170c2c1676..099fc34936fa6 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -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 */
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH 0/2] fs-verity: fix !CONFIG_FS_VERITY case

[Tony Lindgren]

* Eric Biggers <ebiggers@kernel.org> [181211 22:48]:
> Replace the two patches that broken the !CONFIG_FS_VERITY case with
> fixed verisons.

Thanks for fixing these quickly. Mounting ext4 rootfs on SD card
now works again for me with these:

Tested-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 0/2] fs-verity: fix !CONFIG_FS_VERITY case

[Theodore Y. Ts'o]

On Tue, Dec 11, 2018 at 02:46:49PM -0800, Eric Biggers wrote:
> 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:

diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index ea8c418bd7d5..6684bb72bbfc 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -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

Re: [PATCH 0/2] fs-verity: fix !CONFIG_FS_VERITY case

[Eric Biggers]

Hi Ted,

On Tue, Dec 11, 2018 at 09:30:18PM -0500, Theodore Y. Ts'o wrote:
> On Tue, Dec 11, 2018 at 02:46:49PM -0800, Eric Biggers wrote:
> > 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:
> 
> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index ea8c418bd7d5..6684bb72bbfc 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -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

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

Re: [PATCH 0/2] fs-verity: fix !CONFIG_FS_VERITY case

[Theodore Y. Ts'o]

On Tue, Dec 11, 2018 at 06:42:22PM -0800, Eric Biggers wrote:
> 
> 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