[PATCH v3 0/3] xfs: fix NULL deref in log recovery reorder

[PATCH v3 0/3] xfs: fix NULL deref in log recovery reorder

[Weiming Shi]

A crafted on-disk log can commit a transaction whose only item is a bare
transaction header (ri_cnt == 0, ri_buf == NULL).
xlog_recover_reorder_trans() then runs ITEM_TYPE() on it and dereferences
the NULL ri_buf, faulting the kernel at mount time.

v3:
 - patches 1 and 2: picked up the Reviewed-by tags, plus the
   s/encountered/encounter/ fix in patch 2 that Darrick noted
 - patch 3: reworked the changelog per Darrick and Christoph. The empty
   item comes from the len == sizeof(xfs_trans_header) path; the check can
   only run at commit time because a split header may still get regions
   from later ops; and the runtime commit path never emits this, so it is
   only reachable on a crafted log.  It came from an AI-assisted code
   audit of the parser.  Added Cc: stable # v4.3.

Tested on xfs-7.2-fixes with KASAN: the unpatched kernel oopses in
xlog_recover_reorder_trans; the patched kernel fails the mount with
-EFSCORRUPTED and no splat.

Weiming Shi (3):
  xfs: drop ASSERT(0) on unrecognized log item type
  xfs: splice unsorted log items back to the transaction after the loop
  xfs: fail recovery on a committed log item with no regions

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

-- 
2.43.0

[PATCH v3 1/3] xfs: drop ASSERT(0) on unrecognized log item type

[Weiming Shi]

The item type passed to ITEM_TYPE() comes from the on-disk log, so a
fuzzed or crafted image can reach the "unrecognized type" path in
xlog_recover_reorder_trans() and trip its ASSERT(0) on a
CONFIG_XFS_DEBUG kernel.  The -EFSCORRUPTED return handles it fine; drop
the assert.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 fs/xfs/xfs_log_recover.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 5f984bf5698a..a1b373c68f0e 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1912,7 +1912,6 @@ xlog_recover_reorder_trans(
 			xfs_warn(log->l_mp,
 				"%s: unrecognized type of log operation (%d)",
 				__func__, ITEM_TYPE(item));
-			ASSERT(0);
 			/*
 			 * return the remaining items back to the transaction
 			 * item list so they can be freed in caller.
-- 
2.43.0

[PATCH v3 2/3] xfs: splice unsorted log items back to the transaction after the loop

[Weiming Shi]

On error, xlog_recover_reorder_trans() splices the leftover sort_list
items back to trans->r_itemq inside the loop before breaking out.  The
loop tail already splices the per-fate lists back, so do sort_list there
too, guarded by the assert that used to sit after the loop.

No functional change.  It drops the duplicated splice so the next patch
can add another error case without repeating it.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 fs/xfs/xfs_log_recover.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index a1b373c68f0e..103b2a79667b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1912,12 +1912,6 @@ xlog_recover_reorder_trans(
 			xfs_warn(log->l_mp,
 				"%s: unrecognized type of log operation (%d)",
 				__func__, ITEM_TYPE(item));
-			/*
-			 * return the remaining items back to the transaction
-			 * item list so they can be freed in caller.
-			 */
-			if (!list_empty(&sort_list))
-				list_splice_init(&sort_list, &trans->r_itemq);
 			error = -EFSCORRUPTED;
 			break;
 		}
@@ -1945,7 +1939,15 @@ xlog_recover_reorder_trans(
 		}
 	}
 
-	ASSERT(list_empty(&sort_list));
+	/*
+	 * Return the remaining items back to the transaction item list so they
+	 * can be freed in caller.  This should only happen when we encounter
+	 * an error.
+	 */
+	if (!list_empty(&sort_list)) {
+		ASSERT(error);
+		list_splice_init(&sort_list, &trans->r_itemq);
+	}
 	if (!list_empty(&buffer_list))
 		list_splice(&buffer_list, &trans->r_itemq);
 	if (!list_empty(&item_list))
-- 
2.43.0

[PATCH v3] xfs: fail recovery on a committed log item with no regions

[Weiming Shi]

If the first op of a transaction is a bare transaction header
(len == sizeof(struct xfs_trans_header)), xlog_recover_add_to_trans()
adds an item but no region, leaving it on r_itemq with ri_cnt == 0 and
ri_buf == NULL.

The header can be split across op records, so later ops may still add
regions; the item is only invalid if the transaction commits with none.
The runtime commit path never emits such a transaction, so this only
happens on a crafted log.  It came from an AI-assisted code audit of the
recovery parser.

xlog_recover_reorder_trans() calls ITEM_TYPE() on the item, which reads
*(unsigned short *)item->ri_buf[0].iov_base and faults on the NULL
ri_buf.  Reject it there, before the commit handlers that also read
ri_buf[0].

 KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
 RIP: 0010:xlog_recover_reorder_trans (fs/xfs/xfs_log_recover.c:1836)
  xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2043)
  xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2501)
  xlog_do_recovery_pass (fs/xfs/xfs_log_recover.c:3244)
  xlog_recover (fs/xfs/xfs_log_recover.c:3493)
  xfs_log_mount (fs/xfs/xfs_log.c:618)
  xfs_mountfs (fs/xfs/xfs_mount.c:1034)
  xfs_fs_fill_super (fs/xfs/xfs_super.c:1938)
  vfs_get_tree (fs/super.c:1695)
  path_mount (fs/namespace.c:4161)
  __x64_sys_mount (fs/namespace.c:4367)

Fixes: 89cebc847729 ("xfs: validate transaction header length on log recovery")
Cc: <stable@vger.kernel.org> # v4.3
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 fs/xfs/xfs_log_recover.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 103b2a79667b..fdb011e6ef60 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1907,6 +1907,15 @@ xlog_recover_reorder_trans(
 	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
 		enum xlog_recover_reorder	fate = XLOG_REORDER_ITEM_LIST;
 
+		/* a committed item with no regions has a NULL ri_buf[0] */
+		if (!item->ri_cnt || !item->ri_buf) {
+			xfs_warn(log->l_mp,
+				"%s: committed log item has no regions",
+				__func__);
+			error = -EFSCORRUPTED;
+			break;
+		}
+
 		item->ri_ops = xlog_find_item_ops(item);
 		if (!item->ri_ops) {
 			xfs_warn(log->l_mp,
-- 
2.43.0

Re: [PATCH v3] xfs: fail recovery on a committed log item with no regions

[Darrick J. Wong]

On Thu, Jul 02, 2026 at 09:20:00AM -0700, Weiming Shi wrote:
> If the first op of a transaction is a bare transaction header
> (len == sizeof(struct xfs_trans_header)), xlog_recover_add_to_trans()
> adds an item but no region, leaving it on r_itemq with ri_cnt == 0 and
> ri_buf == NULL.
> 
> The header can be split across op records, so later ops may still add
> regions; the item is only invalid if the transaction commits with none.
> The runtime commit path never emits such a transaction, so this only
> happens on a crafted log.  It came from an AI-assisted code audit of the
> recovery parser.
> 
> xlog_recover_reorder_trans() calls ITEM_TYPE() on the item, which reads
> *(unsigned short *)item->ri_buf[0].iov_base and faults on the NULL
> ri_buf.  Reject it there, before the commit handlers that also read
> ri_buf[0].
> 
>  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
>  RIP: 0010:xlog_recover_reorder_trans (fs/xfs/xfs_log_recover.c:1836)
>   xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2043)
>   xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2501)
>   xlog_do_recovery_pass (fs/xfs/xfs_log_recover.c:3244)
>   xlog_recover (fs/xfs/xfs_log_recover.c:3493)
>   xfs_log_mount (fs/xfs/xfs_log.c:618)
>   xfs_mountfs (fs/xfs/xfs_mount.c:1034)
>   xfs_fs_fill_super (fs/xfs/xfs_super.c:1938)
>   vfs_get_tree (fs/super.c:1695)
>   path_mount (fs/namespace.c:4161)
>   __x64_sys_mount (fs/namespace.c:4367)
> 
> Fixes: 89cebc847729 ("xfs: validate transaction header length on log recovery")
> Cc: <stable@vger.kernel.org> # v4.3
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>

Good catch!
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log_recover.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 103b2a79667b..fdb011e6ef60 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1907,6 +1907,15 @@ xlog_recover_reorder_trans(
>  	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
>  		enum xlog_recover_reorder	fate = XLOG_REORDER_ITEM_LIST;
>  
> +		/* a committed item with no regions has a NULL ri_buf[0] */
> +		if (!item->ri_cnt || !item->ri_buf) {
> +			xfs_warn(log->l_mp,
> +				"%s: committed log item has no regions",
> +				__func__);
> +			error = -EFSCORRUPTED;
> +			break;
> +		}
> +
>  		item->ri_ops = xlog_find_item_ops(item);
>  		if (!item->ri_ops) {
>  			xfs_warn(log->l_mp,
> -- 
> 2.43.0
> 
>