linux-kernel - Re: [PATCH v3] xfs: validate log record version against superblock log version

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aRzU0yjBfQ3CjWpp@dread.disaster.area>
Date: Wed, 19 Nov 2025 07:19:31 +1100
From: Dave Chinner <david@...morbit.com>
To: Raphael Pinsonneault-Thibeault <rpthibeault@...il.com>
Cc: cem@...nel.org, djwong@...nel.org, chandanbabu@...nel.org,
	bfoster@...hat.com, linux-xfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kernel-mentees@...ts.linux.dev,
	skhan@...uxfoundation.org,
	syzbot+9f6d080dece587cfdd4c@...kaller.appspotmail.com
Subject: Re: [PATCH v3] xfs: validate log record version against superblock
 log version

On Thu, Nov 13, 2025 at 02:01:13PM -0500, Raphael Pinsonneault-Thibeault wrote:
> Syzbot creates a fuzzed record where xfs_has_logv2() but the
> xlog_rec_header h_version != XLOG_VERSION_2. This causes a
> KASAN: slab-out-of-bounds read in xlog_do_recovery_pass() ->
> xlog_recover_process() -> xlog_cksum().
> 
> Fix by adding a check to xlog_valid_rec_header() to abort journal
> recovery if the xlog_rec_header h_version does not match the super
> block log version.
> 
> A file system with a version 2 log will only ever set
> XLOG_VERSION_2 in its headers (and v1 will only ever set V_1), so if
> there is any mismatch, either the journal or the superblock as been
> corrupted and therefore we abort processing with a -EFSCORRUPTED error
> immediately.
> 
> Reported-by: syzbot+9f6d080dece587cfdd4c@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9f6d080dece587cfdd4c
> Tested-by: syzbot+9f6d080dece587cfdd4c@...kaller.appspotmail.com
> Fixes: 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
> Signed-off-by: Raphael Pinsonneault-Thibeault <rpthibeault@...il.com>
> ---
> changelog
> v1 -> v2: 
> - reject the mount for h_size > XLOG_HEADER_CYCLE_SIZE && !XLOG_VERSION_2
> v2 -> v3:
> - abort journal recovery if the xlog_rec_header h_version does not match 
> the super block log version
> - heavily modify commit description
> 
>  fs/xfs/xfs_log_recover.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e6ed9e09c027..b9a708673965 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2963,6 +2963,14 @@ xlog_valid_rec_header(
>  			__func__, be32_to_cpu(rhead->h_version));
>  		return -EFSCORRUPTED;
>  	}
> +	if (XFS_IS_CORRUPT(log->l_mp, xfs_has_logv2(log->l_mp) !=
> +			   !!(be32_to_cpu(rhead->h_version) & XLOG_VERSION_2))) {
> +		xfs_warn(log->l_mp,
> +"%s: xlog_rec_header h_version (%d) does not match sb log version (%d)",
> +			__func__, be32_to_cpu(rhead->h_version),
> +			xfs_has_logv2(log->l_mp) ? 2 : 1);
> +		return -EFSCORRUPTED;
> +	}

Looks ok, but I can't help but think the validity checks should be
better structured.

At the default error level (LOW), the XFS_IS_CORRUPT() macro emits
the logic expression that failed, the file and line number it is
located at, then dumps the stack. That gives us everything we need
to know about the failure if we do a single validity check per
XFS_IS_CORRUPT() macro like so:

	struct xfs_mount	*mp = log->l_mp;
	u32			h_version = be32_to_cpu(rhead->h_version);

	if (XFS_IS_CORRUPT(mp, !h_version))
		return -EFSCORRUPTED;
	if (XFS_IS_CORRUPT(mp, (h_version & ~XLOG_VERSION_OKBITS))
		return -EFSCORRUPTED;

	/*
	 * We have a known log version, but it also needs to match the superblock
	 * log version feature bits the header can be considered valid.
	 */
	if (xfs_has_logv2(log->l_mp)) {
		if (XFS_IS_CORRUPT(log->l_mp, !(h_version & XLOG_VERSION_2)))
			return -EFSCORRUPTED;
	} else if (XFS_IS_CORRUPT(log->l_mp, !(h_version & XLOG_VERSION_1)))
		return -EFSCORRUPTED;

This avoids the need to both repeatedly recalculate h_version and
emit log messages to indicate what error occurred. It also, IMO,
makes the code cleaner and easier to read.

This pattern is used extensively in on-disk structure verifies in
XFS verifiers, so it makes sense to me to update these on-disk
structure checks to follow that same pattern whilst we are updating
it here...

-Dave.
-- 
Dave Chinner
david@...morbit.com