Re: [PATCH v18 13/15] xfs: check and repair the verity inode flag state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] xfs: unhandled verity validation errors in xchk_inode_setup_verity()
- [High] xfs: leftover post-EOF folios and extents in xrep_inode_verity()
- [Medium] xfs: verity scrub/repair conflicts with corrupt or zapped data fork
- [Low] xfs: wrong inode number passed to xchk_ino_set_corrupt() in xchk_inode()
--

commit 448cf76ee21f3e490894016a838e5f7f0e7783af
Author: Darrick J. Wong <djwong@kernel.org>

xfs: check and repair the verity inode flag state

This commit adds scrub and repair checks for inodes with the fsverity flag
set, attempting to activate fsverity during setup and clearing the verity
flags if activation fails due to a metadata validation error.

> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index cbfe5e6f779df..fb0ae227df54a 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
[ ... ]
> @@ -1765,3 +1767,54 @@ xchk_inode_count_blocks(
>  	return xfs_bmap_count_blocks(sc->tp, sc->ip, whichfork, nextents,
>  			count);
>  }
> +
> +/*
> + * If this inode has S_VERITY set on it, read the verity info. If the reading
> + * fails with anything other than ENOMEM, the file is corrupt, which we can
> + * detect later with fsverity_active.
> + *
> + * Callers must hold the IOLOCK and must not hold the ILOCK of sc->ip because
> + * activation reads inode data.
> + */
> +int
> +xchk_inode_setup_verity(
> +	struct xfs_scrub	*sc)
> +{
> +	int			error;
> +
> +	if (!fsverity_active(VFS_I(sc->ip)))
> +		return 0;
> +
> +	error = fsverity_ensure_verity_info(VFS_I(sc->ip));
> +	switch (error) {
> +	case 0:
> +		/* fsverity is active */
> +		break;
> +	case -ENODATA:
> +	case -EMSGSIZE:
> +	case -EFSCORRUPTED:
> +	case -ERANGE:
> +	case -EBADMSG:
> +		/*
> +		 * The nonzero errno codes above are the error codes that can
> +		 * be returned from fsverity on metadata validation errors.
> +		 */
> +		return 0;
> +	default:
> +		/* runtime errors */
> +		return error;
> +	}

[Severity: High]
Does this switch statement miss several error codes returned by
fsverity_ensure_verity_info() on metadata validation failures, causing
xchk_prepare_iscrub() and xrep_dinode_core() to abort with a runtime error
instead of flagging or repairing the inode?

In fs/verity/open.c, fsverity_get_descriptor() returns -EINVAL when
validate_fsverity_descriptor() fails (for example, unrecognized version,
non-zero reserved bytes, invalid salt_size or sig_size, or when
desc->data_size does not match inode->i_size, including after
xrep_dinode_size() modifies di_size):

	if (!validate_fsverity_descriptor(inode, desc, size)) {
		kfree(desc);
		return -EINVAL;
	}

Similarly, fsverity_create_info() -> fsverity_init_merkle_tree_params()
returns -EINVAL for an unsupported hash algorithm or log_blocksize, and
returns -EFBIG when the Merkle tree depth or hash block count exceeds the
maximum bounds. In addition, xfs_fsverity_get_descriptor() can return
-EFSBADCRC on a block checksum failure, and fsverity_verify_signature() can
return signature verification error codes such as -EKEYREJECTED, -ENOKEY,
-EPERM, or -ENOPKG.

Since those error codes hit the default branch here, won't scrub and repair
abort during setup before xchk_inode() or xrep_inode_verity() can run?

[ ... ]
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index bb8d2bc008e01..1d21758e0df06 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
[ ... ]
> @@ -833,6 +837,9 @@ xchk_inode(
>  	if (S_ISREG(VFS_I(sc->ip)->i_mode))
>  		xchk_inode_check_reflink_iflag(sc, I_INO(sc->ip));
>  
> +	if (xchk_inode_verity_broken(sc->ip))
> +		xchk_ino_set_corrupt(sc, sc->sm->sm_ino);

[Severity: Low]
Should this pass I_INO(sc->ip) instead of sc->sm->sm_ino to
xchk_ino_set_corrupt()?

When scrubbing an open file where the caller passed sc->sm->sm_ino == 0,
xchk_setup_inode() installs the live inode into sc->ip without updating
sc->sm->sm_ino, so passing sc->sm->sm_ino logs inode 0 in
trace_xchk_ino_error().

> diff --git a/fs/xfs/scrub/inode_repair.c b/fs/xfs/scrub/inode_repair.c
> index a9ccf763c0596..e84eaa8178dc7 100644
> --- a/fs/xfs/scrub/inode_repair.c
> +++ b/fs/xfs/scrub/inode_repair.c
[ ... ]
> @@ -1659,6 +1661,10 @@ xrep_dinode_core(
>  	if (iget_error)
>  		return iget_error;
>  
> +	error = xchk_inode_setup_verity(sc);
> +	if (error)
> +		return error;

[Severity: Medium]
What happens if the inode's data fork BMBT is corrupt, or if
xrep_dinode_core() just zapped the data fork via xrep_dinode_zap_forks()?

First, if a verity inode has a valid dinode core but a corrupt out-of-line
data fork BMBT (XFS_DINODE_FMT_BTREE), xchk_prepare_iscrub() ->
xchk_inode_setup_verity() -> xfs_fsverity_get_descriptor() ->
xfs_bmap_last_extent() -> xfs_iread_extents() fails with -EFSCORRUPTED.
xchk_inode_setup_verity() swallows that error, and xchk_inode() then marks
XFS_SCRUB_TYPE_INODE corrupt via xchk_ino_set_corrupt(). When xrep_inode()
Subsequently runs, it sees XFS_SCRUB_OFLAG_CORRUPT and calls
xrep_inode_problems() -> xrep_inode_blockcounts() ->
xchk_inode_count_blocks() before reaching xrep_inode_verity(), which hits
-EFSCORRUPTED again in xfs_iread_extents() and fails the repair before
XFS_SCRUB_TYPE_BMAPBTD can repair the data fork.

Second, if xrep_dinode_core() had to zap the data fork in
xrep_dinode_zap_forks() (setting XFS_SICK_INO_BMBTD_ZAPPED so that
XFS_SCRUB_TYPE_BMAPBTD can later rebuild it from rmapbt), calling
xchk_inode_setup_verity() here on the newly emptied data fork will fail with
-ENODATA, causing xrep_inode_verity() to clear XFS_DIFLAG2_VERITY before
xrep_bmap_data() has a chance to rebuild the data fork.

> @@ -2198,6 +2204,27 @@ xrep_inode_reset_dquots(
>  # define xrep_inode_reset_dquots(sc)		(0)
>  #endif
>  
> +/*
> + * If this file is a fsverity file, xchk_prepare_iscrub or xrep_dinode_core
> + * should have activated it.  If it's still not active, then there's something
> + * wrong with the verity descriptor and we should turn it off.
> + */
> +STATIC int
> +xrep_inode_verity(
> +	struct xfs_scrub	*sc)
> +{
> +	struct inode		*inode = VFS_I(sc->ip);
> +
> +	if (xchk_inode_verity_broken(sc->ip)) {
> +		sc->ip->i_diflags2 &= ~XFS_DIFLAG2_VERITY;
> +		inode_set_flags(inode, 0, S_VERITY);
> +
> +		xfs_trans_log_inode(sc->tp, sc->ip, XFS_ILOG_CORE);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
When xrep_inode_verity() clears XFS_DIFLAG2_VERITY and S_VERITY, does it
also need to invalidate post-EOF pagecache folios and truncate the post-EOF
fsverity metadata extents, similar to xfs_fsverity_delete_metadata()?

During setup, xchk_inode_setup_verity() -> fsverity_ensure_verity_info() ->
xfs_fsverity_get_descriptor() -> fsverity_pagecache_read() reads the
post-EOF Merkle tree and descriptor blocks into inode->i_mapping while
S_VERITY is still set, leaving uptodate folios and written (XFS_EXT_NORM)
extents beyond i_size.

Once XFS_DIFLAG2_VERITY and S_VERITY are cleared without removing those
post-EOF folios and written extents:

1. If the file is later opened for writing and extended via truncate() or
   write(), xfs_zero_range() -> iomap_zero_range() sees IOMAP_MAPPED for the
   post-EOF written extent while inode->i_size is still at the old EOF,
   triggering WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size) in
   iomap_zero_iter().

2. If xfs_free_eofblocks() frees the post-EOF extents first (for example on
   file close), the uptodate post-EOF folios populated by
   fsverity_pagecache_read() remain in inode->i_mapping over the resulting
   IOMAP_HOLE, so a subsequent extending truncate() or write() can expose the
   stale fsverity metadata bytes as file data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102144147.2393369-1-aalbersh@kernel.org?part=13