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

Message

Eric Biggers Dec. 11, 2018, 10:46 p.m. UTC

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

Comments

Tony Lindgren Dec. 12, 2018, 1:37 a.m. UTC | #1

* Eric Biggers <[e-mail]> [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 <[e-mail]>

Theodore Ts'o Dec. 12, 2018, 2:30 a.m. UTC | #2

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

Eric Biggers Dec. 12, 2018, 2:42 a.m. UTC | #3

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

Theodore Ts'o Dec. 12, 2018, 4:07 a.m. UTC | #4

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