linux-kernel - Re: [PATCH v4] 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: <aR67xfAFjuVdbgqq@infradead.org>
Date: Wed, 19 Nov 2025 22:57:09 -0800
From: Christoph Hellwig <hch@...radead.org>
To: Raphael Pinsonneault-Thibeault <rpthibeault@...il.com>
Cc: cem@...nel.org, chandanbabu@...nel.org, djwong@...nel.org,
	bfoster@...hat.com, david@...morbit.com, linux-xfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kernel-mentees@...ts.linux.dev,
	syzbot+9f6d080dece587cfdd4c@...kaller.appspotmail.com
Subject: Re: [PATCH v4] xfs: validate log record version against superblock
 log version

On Wed, Nov 19, 2025 at 10:37:22AM -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 has been
> corrupted and therefore we abort processing with a -EFSCORRUPTED error
> immediately.
> 
> Also, refactor the structure of the validity checks for better
> readability. At the default error level (LOW), XFS_IS_CORRUPT() emits
> the condition that failed, the file and line number it is
> located at, then dumps the stack. This gives us everything we need
> to know about the failure if we do a single validity check per
> XFS_IS_CORRUPT().
> 
> 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
> v3 -> v4: 
> - refactor for readability
> 
>  fs/xfs/xfs_log_recover.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 

> +	if (XFS_IS_CORRUPT(mp, !h_version))
>  		return -EFSCORRUPTED;
> +	if (XFS_IS_CORRUPT(mp, (h_version & ~XLOG_VERSION_OKBITS)))
> +		return -EFSCORRUPTED;
> +
> +	/*
> +	 * the log version is known, but must match the superblock log
> +	 * version feature bits for the header to be considered valid
> +	 */
> +	if (xfs_has_logv2(mp)) {
> +		if (XFS_IS_CORRUPT(mp, !(h_version & XLOG_VERSION_2)))
> +			return -EFSCORRUPTED;
> +	} else if (XFS_IS_CORRUPT(mp, !(h_version & XLOG_VERSION_1)))
> +			return -EFSCORRUPTED;

I'd use the chance to stop pretending h_version is a bitmap.  Given
that only 1 and 2 are defined that's actually still possible.
I.e., kill XLOG_VERSION_OKBITS and replace the four checks in the quoted
code above with:

	/*
	 * The log version must match the superblock:
	 */
	if (xfs_has_logv2(mp)) {
		if (XFS_IS_CORRUPT(mp, h_version != XLOG_VERSION_2))
			return -EFSCORRUPTED;
	} else {
		if (XFS_IS_CORRUPT(mp, h_version != XLOG_VERSION_1))
			return -EFSCORRUPTED;
	}