Re: [PATCH v18 11/15] xfs: add fs-verity ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] xfs: premature S_VERITY set before xfs_free_eofblocks() causes UAF
- [Critical] xfs: fs-verity triggers compound page BUG_ON on large block size fs
- [High] xfs: xfs_fsverity_end_enable() skips rollback cleanup when desc is NULL
- [Medium] xfs: xfs_fileattr_set() rejects FS_VERITY_FL during FS_IOC_SETFLAGS
- [Medium] xfs: xfs_fileattr_set() races with in-progress FS_IOC_ENABLE_VERITY
--

commit 2331d2406515413653c5db56870e5226577de902
Author: Andrey Albershteyn <aalbersh@kernel.org>

xfs: add fs-verity ioctls

This commit exposes the FS_IOC_ENABLE_VERITY, FS_IOC_MEASURE_VERITY, and
FS_IOC_READ_VERITY_METADATA ioctls in xfs_file_ioctl() when the filesystem
has the verity feature enabled.

> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index f81b6e52ac403..37c6a2fae58cc 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c

[ ... ]

> @@ -1575,6 +1576,19 @@ xfs_file_ioctl(
>  	case XFS_IOC_VERIFY_MEDIA:
>  		return xfs_ioc_verify_media(filp, arg);
>  
> +	case FS_IOC_ENABLE_VERITY:
> +		if (!xfs_has_verity(mp))
> +			return -EOPNOTSUPP;
> +		return fsverity_ioctl_enable(filp, arg);

[Severity: High]
When FS_IOC_ENABLE_VERITY fails during Merkle tree construction or
descriptor validation, enable_verity() rolls back by calling
vops->end_enable_verity(filp, NULL, 0, params.tree_size).

In fs/xfs/xfs_fsverity.c:xfs_fsverity_end_enable(), local variable error is
initialized to 0 and the NULL descriptor check jumps directly to out:

	int			error = 0;
	loff_t			range_start = xfs_fsverity_metadata_offset(ip);

	xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);

	/* fs-verity failed, just cleanup */
	if (desc == NULL)
		goto out;
	...
out:
	if (error) {
		int	error2;

		error2 = xfs_fsverity_reset_inode(ip);
		if (error2)
			xfs_alert(ip->i_mount,
"ino 0x%llx failed to clean up new fsverity metadata, err %d",
					I_INO(ip), error2);
	}

	xfs_iflags_clear(ip, XFS_VERITY_CONSTRUCTION);
	return error;

Because error is 0 when desc is NULL, won't xfs_fsverity_reset_inode(ip) be
skipped on rollback, leaving dirty post-EOF Merkle tree folios in
inode->i_mapping and delayed allocation extents in ip->i_df after
XFS_VERITY_CONSTRUCTION is cleared?

[Severity: Medium]
While fsverity_ioctl_enable() -> enable_verity() is building the Merkle
tree, inode_lock(inode) is dropped between xfs_fsverity_begin_enable() and
xfs_fsverity_end_enable(), and XFS_DIFLAG2_VERITY is not set until
xfs_fsverity_end_enable().

During that window, can a concurrent FS_IOC_FSSETXATTR, FS_IOC_SETFLAGS, or
file_setattr call enter xfs_fileattr_set() -> xfs_ioctl_setattr_xflags()
and set XFS_DIFLAG2_DAX or XFS_DIFLAG_REALTIME (including bigrtalloc) on
the inode because neither function checks
xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION)?

[Severity: Medium]
Once FS_IOC_ENABLE_VERITY sets XFS_DIFLAG2_VERITY on an inode,
FS_IOC_GETFLAGS reports FS_VERITY_FL via:

  ioctl_getflags()
    -> xfs_fileattr_get()
      -> xfs_fill_fsxattr()
        -> fileattr_fill_xflags(fa, xfs_ip2xflags(ip))

When userspace tools such as chattr perform a read-modify-write via
FS_IOC_SETFLAGS (where fa->fsx_valid is false), fs/xfs/xfs_ioctl.c:
xfs_fileattr_set() checks:

	if (!fa->fsx_valid) {
		unsigned int allowed = FS_IMMUTABLE_FL | FS_APPEND_FL |
				       FS_NOATIME_FL | FS_NODUMP_FL |
				       FS_SYNC_FL | FS_DAX_FL |
				       FS_PROJINHERIT_FL;
...
		if (xfs_has_asciici(mp))
			allowed |= FS_CASEFOLD_FL;

		if (fa->flags & ~allowed)
			return -EOPNOTSUPP;
	}

Since FS_VERITY_FL is not included in allowed even when XFS_DIFLAG2_VERITY
is already set on the inode, won't FS_IOC_SETFLAGS updates (such as
chattr +i or chattr +a) on verity-enabled files fail with -EOPNOTSUPP?

[Severity: Critical]
On XFS filesystems where sb_blocksize > PAGE_SIZE (or whenever compound
folios are instantiated in inode->i_mapping), xfs_fsverity_begin_enable()
allows enabling verity, and fs/xfs/xfs_fsverity.c:xfs_fsverity_read_merkle()
fetches Merkle tree pages via generic_read_merkle_tree_page():

static struct page *
xfs_fsverity_read_merkle(
	struct inode		*inode,
	pgoff_t			index)
{
	index += xfs_fsverity_metadata_offset(XFS_I(inode)) >> PAGE_SHIFT;

	return generic_read_merkle_tree_page(inode, index);
}

When verify_data_block() and is_hash_block_verified() subsequently call
PageChecked() and SetPageChecked() on the returned struct page belonging to
a compound folio, won't SetPageChecked() trigger
VM_BUG_ON_PGFLAGS(PageCompound(page), page) (defined via PF_NO_COMPOUND) and
corrupt tail page flags?

> +	case FS_IOC_MEASURE_VERITY:
> +		if (!xfs_has_verity(mp))
> +			return -EOPNOTSUPP;
> +		return fsverity_ioctl_measure(filp, arg);
> +	case FS_IOC_READ_VERITY_METADATA:
> +		if (!xfs_has_verity(mp))
> +			return -EOPNOTSUPP;
> +		return fsverity_ioctl_read_metadata(filp, arg);

[Severity: Critical]
In fs/xfs/xfs_fsverity.c:xfs_fsverity_end_enable(), S_VERITY is set on the
VFS inode before calling xfs_free_eofblocks(ip):

	inode_set_flags(inode, S_VERITY, S_VERITY);

	/*
	 * Proactively remove unwritten extents left by COW preallocations and
	 * write preallocation in the merkle tree holes and past descriptor,
	 * and any delayed preallocations
	 */
	error = xfs_free_eofblocks(ip);
out:
	if (error) {
		int	error2;

		error2 = xfs_fsverity_reset_inode(ip);

Because enable_verity() has already hashed struct fsverity_info via
fsverity_set_info() before calling vops->end_enable_verity(), setting
S_VERITY makes fsverity_get_info(inode) return that unrefcounted pointer to
concurrent readers (buffered reads queueing bio verification work,
FS_IOC_MEASURE_VERITY, and FS_IOC_READ_VERITY_METADATA) while
xfs_free_eofblocks(ip) is still running.

If xfs_free_eofblocks(ip) fails, xfs_fsverity_reset_inode(ip) clears
S_VERITY (or leaves S_VERITY set if xfs_fsverity_delete_metadata(ip) also
fails) and enable_verity() removes and RCU-frees struct fsverity_info via
fsverity_remove_info(vi). Can concurrent readers or queued verification work
items then dereference the freed fsverity_info, or can concurrent
FS_IOC_READ_VERITY_METADATA calls invoke xfs_fsverity_get_descriptor() ->
xfs_bmap_last_extent() before xfs_free_eofblocks(ip) removes trailing
post-descriptor preallocations? Should inode_set_flags(inode, S_VERITY,
S_VERITY) only be called after xfs_free_eofblocks(ip) succeeds and rollback
is no longer possible?

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