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

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

[Raphael Pinsonneault-Thibeault]

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

[Christoph Hellwig]

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

[Raphael Pinsonneault-Thibeault]

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

[Darrick J. Wong]

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

[Christoph Hellwig]

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.

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

[Dave Chinner]

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

[Raphael Pinsonneault-Thibeault]

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