[v4] xfs: validate log record version against superblock log version

[PATCH v4] xfs: validate log record version against superblock log version

Posted by Raphael Pinsonneault-Thibeault 1 week, 5 days ago

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9f6d080dece587cfdd4c
Tested-by: syzbot+9f6d080dece587cfdd4c@syzkaller.appspotmail.com
Fixes: 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
Signed-off-by: Raphael Pinsonneault-Thibeault <rpthibeault@gmail.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(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e6ed9e09c027..f5f28755b2ff 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2950,31 +2950,40 @@ xlog_valid_rec_header(
 	xfs_daddr_t		blkno,
 	int			bufsize)
 {
+	struct xfs_mount	*mp = log->l_mp;
+	u32			h_version = be32_to_cpu(rhead->h_version);
 	int			hlen;
 
-	if (XFS_IS_CORRUPT(log->l_mp,
+	if (XFS_IS_CORRUPT(mp,
 			   rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM)))
 		return -EFSCORRUPTED;
-	if (XFS_IS_CORRUPT(log->l_mp,
-			   (!rhead->h_version ||
-			   (be32_to_cpu(rhead->h_version) &
-			    (~XLOG_VERSION_OKBITS))))) {
-		xfs_warn(log->l_mp, "%s: unrecognised log version (%d).",
-			__func__, 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;
+
+	/*
+	 * 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;
 
 	/*
 	 * LR body must have data (or it wouldn't have been written)
 	 * and h_len must not be greater than LR buffer size.
 	 */
 	hlen = be32_to_cpu(rhead->h_len);
-	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > bufsize))
+	if (XFS_IS_CORRUPT(mp, hlen <= 0 || hlen > bufsize))
 		return -EFSCORRUPTED;
 
-	if (XFS_IS_CORRUPT(log->l_mp,
-			   blkno > log->l_logBBsize || blkno > INT_MAX))
+	if (XFS_IS_CORRUPT(mp, blkno > log->l_logBBsize || blkno > INT_MAX))
 		return -EFSCORRUPTED;
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v4] xfs: validate log record version against superblock log version

Posted by Christoph Hellwig 1 week, 4 days ago

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9f6d080dece587cfdd4c
> Tested-by: syzbot+9f6d080dece587cfdd4c@syzkaller.appspotmail.com
> Fixes: 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
> Signed-off-by: Raphael Pinsonneault-Thibeault <rpthibeault@gmail.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;
	}

[PATCH v5] xfs: validate log record version against superblock log version

Posted by Raphael Pinsonneault-Thibeault 1 week ago

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9f6d080dece587cfdd4c
Tested-by: syzbot+9f6d080dece587cfdd4c@syzkaller.appspotmail.com
Fixes: 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
Signed-off-by: Raphael Pinsonneault-Thibeault <rpthibeault@gmail.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
v4 -> v5:
- stop pretending h_version is a bitmap, remove check using
XLOG_VERSION_OKBITS

 fs/xfs/xfs_log_recover.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e6ed9e09c027..2ed94be010d0 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2950,18 +2950,23 @@ xlog_valid_rec_header(
 	xfs_daddr_t		blkno,
 	int			bufsize)
 {
+	struct xfs_mount	*mp = log->l_mp;
+	u32			h_version = be32_to_cpu(rhead->h_version);
 	int			hlen;
 
-	if (XFS_IS_CORRUPT(log->l_mp,
+	if (XFS_IS_CORRUPT(mp,
 			   rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM)))
 		return -EFSCORRUPTED;
-	if (XFS_IS_CORRUPT(log->l_mp,
-			   (!rhead->h_version ||
-			   (be32_to_cpu(rhead->h_version) &
-			    (~XLOG_VERSION_OKBITS))))) {
-		xfs_warn(log->l_mp, "%s: unrecognised log version (%d).",
-			__func__, be32_to_cpu(rhead->h_version));
-		return -EFSCORRUPTED;
+
+	/*
+	 * 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;
 	}
 
 	/*
@@ -2969,12 +2974,12 @@ xlog_valid_rec_header(
 	 * and h_len must not be greater than LR buffer size.
 	 */
 	hlen = be32_to_cpu(rhead->h_len);
-	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > bufsize))
+	if (XFS_IS_CORRUPT(mp, hlen <= 0 || hlen > bufsize))
 		return -EFSCORRUPTED;
 
-	if (XFS_IS_CORRUPT(log->l_mp,
-			   blkno > log->l_logBBsize || blkno > INT_MAX))
+	if (XFS_IS_CORRUPT(mp, blkno > log->l_logBBsize || blkno > INT_MAX))
 		return -EFSCORRUPTED;
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v5] xfs: validate log record version against superblock log version

Posted by Christoph Hellwig 6 days, 19 hours ago

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v5] xfs: validate log record version against superblock log version

Posted by Darrick J. Wong 1 week ago

On Mon, Nov 24, 2025 at 12:47:00PM -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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9f6d080dece587cfdd4c
> Tested-by: syzbot+9f6d080dece587cfdd4c@syzkaller.appspotmail.com
> Fixes: 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
> Signed-off-by: Raphael Pinsonneault-Thibeault <rpthibeault@gmail.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
> v4 -> v5:
> - stop pretending h_version is a bitmap, remove check using
> XLOG_VERSION_OKBITS

Hrmm maybe we ought to reserve XLOG_VERSION==0x3 so that whenever we do
log v3 we don't accidentally write logs with bits that won't be
validated quite right on old kernels?

Though I suppose logv3 would have a separate superblock bit an old
kernel just plain wouldn't mount a logv3 filesystem, let alone look at
its log.

>  fs/xfs/xfs_log_recover.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e6ed9e09c027..2ed94be010d0 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2950,18 +2950,23 @@ xlog_valid_rec_header(
>  	xfs_daddr_t		blkno,
>  	int			bufsize)
>  {
> +	struct xfs_mount	*mp = log->l_mp;
> +	u32			h_version = be32_to_cpu(rhead->h_version);
>  	int			hlen;
>  
> -	if (XFS_IS_CORRUPT(log->l_mp,
> +	if (XFS_IS_CORRUPT(mp,
>  			   rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM)))
>  		return -EFSCORRUPTED;
> -	if (XFS_IS_CORRUPT(log->l_mp,
> -			   (!rhead->h_version ||
> -			   (be32_to_cpu(rhead->h_version) &
> -			    (~XLOG_VERSION_OKBITS))))) {
> -		xfs_warn(log->l_mp, "%s: unrecognised log version (%d).",
> -			__func__, be32_to_cpu(rhead->h_version));
> -		return -EFSCORRUPTED;
> +
> +	/*
> +	 * The log version must match the superblock
> +	 */
> +	if (xfs_has_logv2(mp)) {
> +		if (XFS_IS_CORRUPT(mp, h_version != XLOG_VERSION_2))

Being pedantic here, but the kernel cpu_to_be32 wrappers are magic in
that they compile to byteswapped constants so you can avoid the runtime
overhead of byteswapping rhead->h_version by doing:

	if (XFS_IS_CORRUPT(mp,
	    rhead->h_version != cpu_to_be32(XLOG_VERSION_2)))
		return -EFSCORRUPTED;

But seeing as this is log validation for recovery, I think the
performance implications are vanishingly small.

I suppose if we /really/ want to be pedantic then the change of bitmask
test to direct comparison ought to be a separate patch in case someone
some day ends up bisecting a log recovery problem to this patch.

Either way,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


> +			return -EFSCORRUPTED;
> +	} else {
> +		if (XFS_IS_CORRUPT(mp, h_version != XLOG_VERSION_1))
> +			return -EFSCORRUPTED;
>  	}
>  
>  	/*
> @@ -2969,12 +2974,12 @@ xlog_valid_rec_header(
>  	 * and h_len must not be greater than LR buffer size.
>  	 */
>  	hlen = be32_to_cpu(rhead->h_len);
> -	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > bufsize))
> +	if (XFS_IS_CORRUPT(mp, hlen <= 0 || hlen > bufsize))
>  		return -EFSCORRUPTED;
>  
> -	if (XFS_IS_CORRUPT(log->l_mp,
> -			   blkno > log->l_logBBsize || blkno > INT_MAX))
> +	if (XFS_IS_CORRUPT(mp, blkno > log->l_logBBsize || blkno > INT_MAX))
>  		return -EFSCORRUPTED;
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
>

Re: [PATCH v5] xfs: validate log record version against superblock log version

Posted by Christoph Hellwig 6 days, 19 hours ago

> Hrmm maybe we ought to reserve XLOG_VERSION==0x3 so that whenever we do
> log v3 we don't accidentally write logs with bits that won't be
> validated quite right on old kernels?

Why do we need to reserve that?  The code checks for either 1 or 2
right now based on the log feature flag.  If we add a v3 log we'll
have to ammend this, but reservations won't help with that.

> > +	if (xfs_has_logv2(mp)) {
> > +		if (XFS_IS_CORRUPT(mp, h_version != XLOG_VERSION_2))
> 
> Being pedantic here, but the kernel cpu_to_be32 wrappers are magic in
> that they compile to byteswapped constants so you can avoid the runtime
> overhead of byteswapping rhead->h_version by doing:
> 
> 	if (XFS_IS_CORRUPT(mp,
> 	    rhead->h_version != cpu_to_be32(XLOG_VERSION_2)))
> 		return -EFSCORRUPTED;
> 
> But seeing as this is log validation for recovery, I think the
> performance implications are vanishingly small.

Yes.

Re: [PATCH v5] xfs: validate log record version against superblock log version

Posted by Darrick J. Wong 6 days, 8 hours ago

On Mon, Nov 24, 2025 at 10:31:22PM -0800, Christoph Hellwig wrote:
> > Hrmm maybe we ought to reserve XLOG_VERSION==0x3 so that whenever we do
> > log v3 we don't accidentally write logs with bits that won't be
> > validated quite right on old kernels?
> 
> Why do we need to reserve that?  The code checks for either 1 or 2
> right now based on the log feature flag.  If we add a v3 log we'll
> have to ammend this, but reservations won't help with that.

Yeah, I suppose you're right -- log v3 will require a new sb feature
bit, and that's good enough.

--D

> > > +	if (xfs_has_logv2(mp)) {
> > > +		if (XFS_IS_CORRUPT(mp, h_version != XLOG_VERSION_2))
> > 
> > Being pedantic here, but the kernel cpu_to_be32 wrappers are magic in
> > that they compile to byteswapped constants so you can avoid the runtime
> > overhead of byteswapping rhead->h_version by doing:
> > 
> > 	if (XFS_IS_CORRUPT(mp,
> > 	    rhead->h_version != cpu_to_be32(XLOG_VERSION_2)))
> > 		return -EFSCORRUPTED;
> > 
> > But seeing as this is log validation for recovery, I think the
> > performance implications are vanishingly small.
> 
> Yes.
> 
>

Re: [PATCH v4] xfs: validate log record version against superblock log version

Posted by Dave Chinner 1 week, 5 days ago

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9f6d080dece587cfdd4c
> Tested-by: syzbot+9f6d080dece587cfdd4c@syzkaller.appspotmail.com
> Fixes: 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
> Signed-off-by: Raphael Pinsonneault-Thibeault <rpthibeault@gmail.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(-)

Looks good to me now.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com