On Sat, Jun 15, 2019 at 11:08:21AM -0400, Theodore Ts'o wrote:

On Thu, Jun 06, 2019 at 08:51:59AM -0700, Eric Biggers wrote:

quoted

From: Eric Biggers <redacted>

Add a function for filesystems to call to implement the
FS_IOC_ENABLE_VERITY ioctl.  This ioctl enables fs-verity on a file.

See the "FS_IOC_ENABLE_VERITY" section of
Documentation/filesystems/fsverity.rst for the documentation.

Signed-off-by: Eric Biggers <redacted>

quoted

diff --git a/fs/verity/enable.c b/fs/verity/enable.c
new file mode 100644
index 000000000000..7e7ef9d3c376
--- /dev/null
+++ b/fs/verity/enable.c
+	/* Tell the filesystem to finish enabling verity on the file */
+	err = vops->end_enable_verity(filp, desc, desc_size, params.tree_size);
+	if (err) {
+		fsverity_err(inode, "%ps() failed with err %d",
+			     vops->end_enable_verity, err);
+		fsverity_free_info(vi);
+	} else {
+		/* Successfully enabled verity */
+
+		WARN_ON(!IS_VERITY(inode));
+
+		/*
+		 * Readers can start using ->i_verity_info immediately, so it
+		 * can't be rolled back once set.  So don't set it until just
+		 * after the filesystem has successfully enabled verity.
+		 */
+		fsverity_set_info(inode, vi);
+	}

If end_enable_Verity() retuns success, and IS_VERITY is not set, I
would think that we should report the error via fsverity_err() and
return an error to userspace, and *not* call fsverity_set_info().  I
don't think the stack trace printed by WARN_ON is going to very
interesting, since the call path which gets us to enable_verity() is
not going to be surprising.

I want to keep it as WARN_ON() because if it happens it's a kernel bug, and
WARNs are reported as bugs by automated tools.  But I can do the following so it
returns an error code too:

@@ -229,11 +235,12 @@ static int enable_verity(struct file *filp,
 		fsverity_err(inode, "%ps() failed with err %d",
 			     vops->end_enable_verity, err);
 		fsverity_free_info(vi);
+	} else if (WARN_ON(!IS_VERITY(inode))) {
+		err = -EINVAL;
+		fsverity_free_info(vi);
 	} else {
 		/* Successfully enabled verity */
 
-		WARN_ON(!IS_VERITY(inode));
-
 		/*
 		 * Readers can start using ->i_verity_info immediately, so it
 		 * can't be rolled back once set.  So don't set it until just

quoted

+
+	if (inode->i_size <= 0) {
+		err = -EINVAL;
+		goto out_unlock;
+	}

How hard would it be to support fsverity for zero-length files?  There
would be no Merkle tree, but there still would be an fsverity header
file on which we can calculate a checksum for the digital signature.

     	      	     	       	 - Ted

Empty files would have to be special-cased, e.g. defining the root hash to be
all 0's, since there are no blocks to checksum.  It would be straightforward,
but it would still be a special case, e.g.:

diff --git a/fs/verity/enable.c b/fs/verity/enable.c
index ee9dd578e59fb..e859a2b6a4310 100644
--- a/fs/verity/enable.c
+++ b/fs/verity/enable.c

@@ -112,6 +112,12 @@ static int build_merkle_tree(struct inode *inode,
 	unsigned int level;
 	int err = -ENOMEM;
 
+	if (inode->i_size == 0) {
+		/* Empty file is a special case; root hash is all 0's */
+		memset(root_hash, 0, params->digest_size);
+		return 0;
+	}
+

On the other hand, *not* supporting empty files is a special case from the
user's point of view.  It means that fs-verity isn't supported on every possible
file.  Thinking about it, that's probably worse than having a special case in
the *implementation*.

So now I'm leaning towards changing it to support empty files.

- Eric