xfs: ensure log recovery buffer is resized to avoid OOB

[PATCH] xfs: ensure log recovery buffer is resized to avoid OOB

Posted by Raphael Pinsonneault-Thibeault 2 months, 3 weeks ago

In xlog_do_recovery_pass(),
commit 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
added a fix to take the corrected h_size (from the xfsprogs bug
workaround) into consideration for the log recovery buffer calculation.
Without it, we would still allocate the buffer based on the incorrect
on-disk size.

However, in a scenario similar to 45cf976008dd, syzbot creates a fuzzed
record where xfs_has_logv2() but the xlog_rec_header h_version !=
XLOG_VERSION_2. Meaning, we skip the log recover buffer calculation
fix and allocate the buffer based on the incorrect on-disk size. Hence,
a KASAN: slab-out-of-bounds read in xlog_do_recovery_pass() ->
xlog_recover_process() -> xlog_cksum().

Fix by removing the check for xlog_rec_header h_version, since the code
is already within the if(xfs_has_logv2) path. The CRC checksum will
reject the bad record anyway, this fix is to ensure we can read the
entire buffer without an OOB.

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>
---

Can xfs_has_logv2() and xlog_rec_header h_version ever disagree?

From my understanding, whenever the h_version is set (e.g. in
xlog_add_record()), it is set to 2 when xfs_has_logv2(), and 1
otherwise.

 fs/xfs/xfs_log_recover.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e6ed9e09c027..08d7b25c4ab1 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3064,8 +3064,7 @@ xlog_do_recovery_pass(
 		 * still allocate the buffer based on the incorrect on-disk
 		 * size.
 		 */
-		if (h_size > XLOG_HEADER_CYCLE_SIZE &&
-		    (rhead->h_version & cpu_to_be32(XLOG_VERSION_2))) {
+		if (h_size > XLOG_HEADER_CYCLE_SIZE) {
 			hblks = DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
 			if (hblks > 1) {
 				kvfree(hbp);
-- 
2.43.0

Re: [PATCH] xfs: ensure log recovery buffer is resized to avoid OOB

Posted by Dave Chinner 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 09:10:34AM -0500, Raphael Pinsonneault-Thibeault wrote:
> In xlog_do_recovery_pass(),
> commit 45cf976008dd ("xfs: fix log recovery buffer allocation for the legacy h_size fixup")
> added a fix to take the corrected h_size (from the xfsprogs bug
> workaround) into consideration for the log recovery buffer calculation.
> Without it, we would still allocate the buffer based on the incorrect
> on-disk size.
> 
> However, in a scenario similar to 45cf976008dd, syzbot creates a fuzzed
> record where xfs_has_logv2() but the xlog_rec_header h_version !=
> XLOG_VERSION_2.

We should abort journal recovery at that point because the record
header is corrupt and we can't trust it.

i.e. A filesytem with a version 2 log will only ever set XLOG_VERSION_2
in it's headers (and v1 will only ever set V_1), so if there is a
mismatch something has gone wrong and we should stop processing the
journal immediately.

Otherwise, stuff taht assumes the version flags are coherenti like
this...

> Meaning, we skip the log recover buffer calculation
> fix and allocate the buffer based on the incorrect on-disk size. Hence,
> a KASAN: slab-out-of-bounds read in xlog_do_recovery_pass() ->
> xlog_recover_process() -> xlog_cksum().

... goes wrong.

....

> Can xfs_has_logv2() and xlog_rec_header h_version ever disagree?

No. As per above, if they differ, either the journal or the
superblock has been corrupted and we need to abort processing with a
-EFSCORRUPTED error immediately.

That's the change that needs to be made here - xlog_valid_rec_header()
should validate that the header and sb log versions match, not just
that the record header only has "known" version bits set.

If we validate this up front, then the rest of the code can then
safely assume that xfs_has_logv2() and xlog_rec_header h_version are
coherent and correct and so won't be exposed to bugs related to an
undetected mismatch of various version fields...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Posted by Raphael Pinsonneault-Thibeault 2 months, 3 weeks 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 as been
corrupted and therefore we abort processing with a -EFSCORRUPTED error
immediately.

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
- 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;
+	}
 
 	/*
 	 * LR body must have data (or it wouldn't have been written)
-- 
2.43.0

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

Posted by Dave Chinner 2 months, 3 weeks ago

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@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
> - 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@fromorbit.com

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

Posted by Raphael Pinsonneault-Thibeault 2 months, 2 weeks 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 2 months, 2 weeks 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 2 months, 2 weeks 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 2 months, 2 weeks 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 2 months, 2 weeks 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 2 months, 2 weeks 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 2 months, 2 weeks 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 2 months, 2 weeks 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

Re: [PATCH] xfs: ensure log recovery buffer is resized to avoid OOB

Posted by Christoph Hellwig 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 09:10:34AM -0500, Raphael Pinsonneault-Thibeault wrote:
> Fix by removing the check for xlog_rec_header h_version, since the code
> is already within the if(xfs_has_logv2) path. The CRC checksum will
> reject the bad record anyway, this fix is to ensure we can read the
> entire buffer without an OOB.

Thanks for the fix and the very detailed commit message explaining
the logic.  I think this should work, but I suspect the better fix
would be to just reject the mount for

	h_size > XLOG_HEADER_CYCLE_SIZE && !XLOG_VERSION_2

because the larger h_size can't work for v1 logs, and the log stripe
unit adjustment is also a v2 feature, so it really should not have
been applied even accidentally in mkfs.

> Can xfs_has_logv2() and xlog_rec_header h_version ever disagree?

They should not, but I'm pretty sure if we give syzbot enough time
it'll craft an image doing that :)  So we better add sanity checks
for that now.

[PATCH] xfs: reject log records with v2 size but v1 header version to avoid OOB

Posted by Raphael Pinsonneault-Thibeault 2 months, 3 weeks ago

In xlog_do_recovery_pass(),
commit 45cf976008dd ("xfs: fix log recovery buffer allocation for the
legacy h_size fixup")
added a fix to take the corrected h_size (from the xfsprogs bug
workaround) into consideration for the log recovery buffer calculation.
Without it, we would still allocate the buffer based on the incorrect
on-disk size.

However, in a scenario similar to 45cf976008dd, syzbot creates a fuzzed
record where xfs_has_logv2() but the xlog_rec_header h_version !=
XLOG_VERSION_2. Meaning, we skip the log recover buffer calculation
fix and allocate the buffer based on the incorrect on-disk size. Hence,
a KASAN: slab-out-of-bounds read in xlog_do_recovery_pass() ->
xlog_recover_process() -> xlog_cksum().

Fix by rejecting the record header for
h_size > XLOG_HEADER_CYCLE_SIZE && !XLOG_VERSION_2
since the larger h_size cannot work for v1 logs, and the log stripe unit
adjustment is only a v2 feature.

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
- update commit subject and message

 fs/xfs/xfs_log_recover.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e6ed9e09c027..99a903e01869 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3064,8 +3064,12 @@ xlog_do_recovery_pass(
 		 * still allocate the buffer based on the incorrect on-disk
 		 * size.
 		 */
-		if (h_size > XLOG_HEADER_CYCLE_SIZE &&
-		    (rhead->h_version & cpu_to_be32(XLOG_VERSION_2))) {
+		if (h_size > XLOG_HEADER_CYCLE_SIZE) {
+			if (!(rhead->h_version & cpu_to_be32(XLOG_VERSION_2))) {
+				error = -EFSCORRUPTED;
+				goto bread_err1;
+			}
+
 			hblks = DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
 			if (hblks > 1) {
 				kvfree(hbp);
-- 
2.43.0

Re: [PATCH] xfs: reject log records with v2 size but v1 header version to avoid OOB

Posted by Darrick J. Wong 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 01:18:18PM -0500, Raphael Pinsonneault-Thibeault wrote:
> In xlog_do_recovery_pass(),
> commit 45cf976008dd ("xfs: fix log recovery buffer allocation for the
> legacy h_size fixup")
> added a fix to take the corrected h_size (from the xfsprogs bug
> workaround) into consideration for the log recovery buffer calculation.
> Without it, we would still allocate the buffer based on the incorrect
> on-disk size.
> 
> However, in a scenario similar to 45cf976008dd, syzbot creates a fuzzed
> record where xfs_has_logv2() but the xlog_rec_header h_version !=
> XLOG_VERSION_2. Meaning, we skip the log recover buffer calculation
> fix and allocate the buffer based on the incorrect on-disk size. Hence,
> a KASAN: slab-out-of-bounds read in xlog_do_recovery_pass() ->
> xlog_recover_process() -> xlog_cksum().
> 
> Fix by rejecting the record header for
> h_size > XLOG_HEADER_CYCLE_SIZE && !XLOG_VERSION_2
> since the larger h_size cannot work for v1 logs, and the log stripe unit
> adjustment is only a v2 feature.
> 
> 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
> - update commit subject and message
> 
>  fs/xfs/xfs_log_recover.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e6ed9e09c027..99a903e01869 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3064,8 +3064,12 @@ xlog_do_recovery_pass(
>  		 * still allocate the buffer based on the incorrect on-disk
>  		 * size.
>  		 */
> -		if (h_size > XLOG_HEADER_CYCLE_SIZE &&
> -		    (rhead->h_version & cpu_to_be32(XLOG_VERSION_2))) {

Just out of curiosity, why is this a bit flag test?  Did XFS ever emit a
log record with both XLOG_VERSION_2 *and* XLOG_VERSION_1 set?  The code
that writes new log records only sets h_version to 1 or 2, not 3.

(I can't tell if this is a hysterical raisins compatibility thing, or
just bugs)

--D

> +		if (h_size > XLOG_HEADER_CYCLE_SIZE) {
> +			if (!(rhead->h_version & cpu_to_be32(XLOG_VERSION_2))) {
> +				error = -EFSCORRUPTED;
> +				goto bread_err1;
> +			}
> +
>  			hblks = DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
>  			if (hblks > 1) {
>  				kvfree(hbp);
> -- 
> 2.43.0
> 
>

Re: [PATCH] xfs: reject log records with v2 size but v1 header version to avoid OOB

Posted by Christoph Hellwig 2 months, 3 weeks ago

On Wed, Nov 12, 2025 at 10:45:04AM -0800, Darrick J. Wong wrote:
> > @@ -3064,8 +3064,12 @@ xlog_do_recovery_pass(
> >  		 * still allocate the buffer based on the incorrect on-disk
> >  		 * size.
> >  		 */
> > -		if (h_size > XLOG_HEADER_CYCLE_SIZE &&
> > -		    (rhead->h_version & cpu_to_be32(XLOG_VERSION_2))) {
> 
> Just out of curiosity, why is this a bit flag test?  Did XFS ever emit a
> log record with both XLOG_VERSION_2 *and* XLOG_VERSION_1 set?  The code
> that writes new log records only sets h_version to 1 or 2, not 3.

Yeah.  This particular instance got added by me, but it is a copy and
paste from xlog_logrec_hblks, which again consolidate multiple chunks
of this style of code, which were moved around a few times.

I think originally this came from Nathan fixing this:

-               if ((h_version && XLOG_VERSION_2) &&
+
+               if ((h_version & XLOG_VERSION_2) &&

or in other words, this was a mess all the way back.