diff mbox series

[11/13] fsverity: report validation errors back to the filesystem

Message ID 171175868048.1987804.2771715174385554090.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series [01/13] fs: add FS_XFLAG_VERITY for verity files | expand

Commit Message

Darrick J. Wong March 30, 2024, 12:35 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Provide a new function call so that validation errors can be reported
back to the filesystem.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/verity/verify.c       |   14 +++++++++++++-
 include/linux/fsverity.h |   11 +++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

Comments

Eric Biggers April 5, 2024, 3:09 a.m. UTC | #1 
On Fri, Mar 29, 2024 at 05:35:32PM -0700, Darrick J. Wong wrote:
> +	/**
> +	 * Notify the filesystem that file data validation failed
> +	 *
> +	 * @inode: the inode being validated
> +	 * @pos: the file position of the invalid data
> +	 * @len: the length of the invalid data
> +	 *
> +	 * This is called when fs-verity cannot validate the file contents.
> +	 */
> +	void (*fail_validation)(struct inode *inode, loff_t pos, size_t len);

There is a difference between the file actually being corrupt (mismatching
hashes) and other problems like disk errors reading from the Merkle tree.
"Validation failed" is a bit ambiguous, and "cannot validate the file contents"
even more so.  Do you want only file corruption errors?  If so it may be a good
idea to call this 'file_corrupt', which would be consistent with the
"FILE CORRUPTED" error message in fs/verity/verify.c.  Or do you actually want
all errors?  Either way, it needs to be clarified what is actually meant.

- Eric
Darrick J. Wong April 24, 2024, 6:18 p.m. UTC | #2 
On Thu, Apr 04, 2024 at 11:09:11PM -0400, Eric Biggers wrote:
> On Fri, Mar 29, 2024 at 05:35:32PM -0700, Darrick J. Wong wrote:
> > +	/**
> > +	 * Notify the filesystem that file data validation failed
> > +	 *
> > +	 * @inode: the inode being validated
> > +	 * @pos: the file position of the invalid data
> > +	 * @len: the length of the invalid data
> > +	 *
> > +	 * This is called when fs-verity cannot validate the file contents.
> > +	 */
> > +	void (*fail_validation)(struct inode *inode, loff_t pos, size_t len);
> 
> There is a difference between the file actually being corrupt (mismatching
> hashes) and other problems like disk errors reading from the Merkle tree.
> "Validation failed" is a bit ambiguous, and "cannot validate the file contents"
> even more so.  Do you want only file corruption errors?  If so it may be a good
> idea to call this 'file_corrupt', which would be consistent with the
> "FILE CORRUPTED" error message in fs/verity/verify.c.  Or do you actually want
> all errors?  Either way, it needs to be clarified what is actually meant.

I only want actual file corruption errors -- XFS can handle disk errors
from reading merkle tree blocks on its own.  I'll change this to
file_corrupt.  How's this?

	/**
	 * Notify the filesystem that file data is corrupt.
	 *
	 * @inode: the inode being validated
	 * @pos: the file position of the invalid data
	 * @len: the length of the invalid data
	 *
	 * This function is called when fs-verity cannot validate the file
	 * contents against the merkle tree hashes and logs a FILE CORRUPTED
	 * error message.
	 */
	void (*file_corrupt)(struct inode *inode, loff_t pos, size_t len);

--D

> - Eric
>
Eric Biggers April 24, 2024, 6:52 p.m. UTC | #3 
On Wed, Apr 24, 2024 at 11:18:26AM -0700, Darrick J. Wong wrote:
> On Thu, Apr 04, 2024 at 11:09:11PM -0400, Eric Biggers wrote:
> > On Fri, Mar 29, 2024 at 05:35:32PM -0700, Darrick J. Wong wrote:
> > > +	/**
> > > +	 * Notify the filesystem that file data validation failed
> > > +	 *
> > > +	 * @inode: the inode being validated
> > > +	 * @pos: the file position of the invalid data
> > > +	 * @len: the length of the invalid data
> > > +	 *
> > > +	 * This is called when fs-verity cannot validate the file contents.
> > > +	 */
> > > +	void (*fail_validation)(struct inode *inode, loff_t pos, size_t len);
> > 
> > There is a difference between the file actually being corrupt (mismatching
> > hashes) and other problems like disk errors reading from the Merkle tree.
> > "Validation failed" is a bit ambiguous, and "cannot validate the file contents"
> > even more so.  Do you want only file corruption errors?  If so it may be a good
> > idea to call this 'file_corrupt', which would be consistent with the
> > "FILE CORRUPTED" error message in fs/verity/verify.c.  Or do you actually want
> > all errors?  Either way, it needs to be clarified what is actually meant.
> 
> I only want actual file corruption errors -- XFS can handle disk errors
> from reading merkle tree blocks on its own.  I'll change this to
> file_corrupt.  How's this?
> 
> 	/**
> 	 * Notify the filesystem that file data is corrupt.
> 	 *
> 	 * @inode: the inode being validated
> 	 * @pos: the file position of the invalid data
> 	 * @len: the length of the invalid data
> 	 *
> 	 * This function is called when fs-verity cannot validate the file
> 	 * contents against the merkle tree hashes and logs a FILE CORRUPTED
> 	 * error message.
> 	 */
> 	void (*file_corrupt)(struct inode *inode, loff_t pos, size_t len);

It looks good except for the last sentence, which still has the potentially
misleading "cannot validate the file contents" wording.  How about something
like the following:

"This function is called when fs-verity detects that a portion of a file's data
is inconsistent with the Merkle tree, or a Merkle tree block needed to validate
the data is inconsistent with the level above it."

- Eric
Darrick J. Wong April 24, 2024, 7:03 p.m. UTC | #4 
On Wed, Apr 24, 2024 at 06:52:30PM +0000, Eric Biggers wrote:
> On Wed, Apr 24, 2024 at 11:18:26AM -0700, Darrick J. Wong wrote:
> > On Thu, Apr 04, 2024 at 11:09:11PM -0400, Eric Biggers wrote:
> > > On Fri, Mar 29, 2024 at 05:35:32PM -0700, Darrick J. Wong wrote:
> > > > +	/**
> > > > +	 * Notify the filesystem that file data validation failed
> > > > +	 *
> > > > +	 * @inode: the inode being validated
> > > > +	 * @pos: the file position of the invalid data
> > > > +	 * @len: the length of the invalid data
> > > > +	 *
> > > > +	 * This is called when fs-verity cannot validate the file contents.
> > > > +	 */
> > > > +	void (*fail_validation)(struct inode *inode, loff_t pos, size_t len);
> > > 
> > > There is a difference between the file actually being corrupt (mismatching
> > > hashes) and other problems like disk errors reading from the Merkle tree.
> > > "Validation failed" is a bit ambiguous, and "cannot validate the file contents"
> > > even more so.  Do you want only file corruption errors?  If so it may be a good
> > > idea to call this 'file_corrupt', which would be consistent with the
> > > "FILE CORRUPTED" error message in fs/verity/verify.c.  Or do you actually want
> > > all errors?  Either way, it needs to be clarified what is actually meant.
> > 
> > I only want actual file corruption errors -- XFS can handle disk errors
> > from reading merkle tree blocks on its own.  I'll change this to
> > file_corrupt.  How's this?
> > 
> > 	/**
> > 	 * Notify the filesystem that file data is corrupt.
> > 	 *
> > 	 * @inode: the inode being validated
> > 	 * @pos: the file position of the invalid data
> > 	 * @len: the length of the invalid data
> > 	 *
> > 	 * This function is called when fs-verity cannot validate the file
> > 	 * contents against the merkle tree hashes and logs a FILE CORRUPTED
> > 	 * error message.
> > 	 */
> > 	void (*file_corrupt)(struct inode *inode, loff_t pos, size_t len);
> 
> It looks good except for the last sentence, which still has the potentially
> misleading "cannot validate the file contents" wording.  How about something
> like the following:
> 
> "This function is called when fs-verity detects that a portion of a file's data
> is inconsistent with the Merkle tree, or a Merkle tree block needed to validate
> the data is inconsistent with the level above it."

Much better!  I'll change it to that, thank you for the suggestion.

--D

> - Eric
>
diff mbox series

Patch

diff --git a/fs/verity/verify.c b/fs/verity/verify.c
index 99b1529bbb50b..4acfd02b0e42d 100644
--- a/fs/verity/verify.c
+++ b/fs/verity/verify.c
@@ -258,6 +258,15 @@  verify_data_block(struct inode *inode, struct fsverity_info *vi,
 	return false;
 }
 
+static void fsverity_fail_validation(struct inode *inode, loff_t pos,
+				     size_t len)
+{
+	const struct fsverity_operations *vops = inode->i_sb->s_vop;
+
+	if (vops->fail_validation)
+		vops->fail_validation(inode, pos, len);
+}
+
 static bool
 verify_data_blocks(struct folio *data_folio, size_t len, size_t offset,
 		   unsigned long max_ra_bytes)
@@ -280,8 +289,11 @@  verify_data_blocks(struct folio *data_folio, size_t len, size_t offset,
 		valid = verify_data_block(inode, vi, data, pos + offset,
 					  max_ra_bytes);
 		kunmap_local(data);
-		if (!valid)
+		if (!valid) {
+			fsverity_fail_validation(inode, pos + offset,
+						 block_size);
 			return false;
+		}
 		offset += block_size;
 		len -= block_size;
 	} while (len);
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 761a0b76eefec..dcf9d9cffcb9f 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -236,6 +236,17 @@  struct fsverity_operations {
 	 * be implemented.
 	 */
 	void (*drop_merkle_tree_block)(struct fsverity_blockbuf *block);
+
+	/**
+	 * Notify the filesystem that file data validation failed
+	 *
+	 * @inode: the inode being validated
+	 * @pos: the file position of the invalid data
+	 * @len: the length of the invalid data
+	 *
+	 * This is called when fs-verity cannot validate the file contents.
+	 */
+	void (*fail_validation)(struct inode *inode, loff_t pos, size_t len);
 };
 
 #ifdef CONFIG_FS_VERITY