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

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

[Weiming Shi]

xlog_recover_add_to_trans() turns the first op of a transaction into a
recovery item.  If that op is a bare transaction header
(len == sizeof(struct xfs_trans_header)), xlog_recover_add_item() adds
an item but attaches no region, so it sits on r_itemq with ri_cnt == 0
and ri_buf == NULL.

When the following op is a commit, xlog_recover_reorder_trans() runs
ITEM_TYPE() on each item to look up its ops vector.  ITEM_TYPE() reads
*(unsigned short *)item->ri_buf[0].iov_base, which faults on the
NULL ri_buf.  The commit handlers dereference ri_buf[0] too, so reorder
is the first place that trips over it.

This is reachable at mount time from a crafted image whose log holds the
op sequence START_TRANS / bare-header / COMMIT_TRANS:

 Oops: general protection fault, probably for non-canonical address
       0xdffffc0000000000
 KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
 RIP: 0010:xlog_recover_reorder_trans (fs/xfs/xfs_log_recover.c:1836)
 Call Trace:
  xlog_recover_reorder_trans (fs/xfs/xfs_log_recover.c:1836)
  xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2043)
  xlog_recovery_process_trans (fs/xfs/xfs_log_recover.c:2308)
  xlog_recover_process_ophdr (fs/xfs/xfs_log_recover.c:2454)
  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)
  get_tree_bdev_flags (fs/super.c:1635)
  vfs_get_tree (fs/super.c:1695)
  path_mount (fs/namespace.c:4161)
  __x64_sys_mount (fs/namespace.c:4367)

xfs_log_recover.c:1836 is the ITEM_TYPE() dereference inside
xlog_find_item_ops().

A committed item always has at least its format descriptor in ri_buf[0],
so an item with no regions means the log is corrupt.  Reject it with
-EFSCORRUPTED, like the unrecognised item type just below.

Fixes: 89cebc847729 ("xfs: validate transaction header length on log recovery")
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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 09e6678ca487..ccff436612ee 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1906,6 +1906,18 @@ xlog_recover_reorder_trans(
 	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
 		enum xlog_recover_reorder	fate = XLOG_REORDER_ITEM_LIST;
 
+		/* corrupt log: an item with no regions has a NULL ri_buf */
+		if (!item->ri_cnt || !item->ri_buf) {
+			xfs_warn(log->l_mp,
+				"%s: committed log item has no regions",
+				__func__);
+			ASSERT(0);
+			if (!list_empty(&sort_list))
+				list_splice_init(&sort_list, &trans->r_itemq);
+			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] xfs: fail recovery on a committed log item with no regions

[Christoph Hellwig]

On Mon, Jun 29, 2026 at 09:24:53AM -0700, Weiming Shi wrote:
> xlog_recover_add_to_trans() turns the first op of a transaction into a
> recovery item.  If that op is a bare transaction header
> (len == sizeof(struct xfs_trans_header)), xlog_recover_add_item() adds
> an item but attaches no region, so it sits on r_itemq with ri_cnt == 0
> and ri_buf == NULL.
> 
> When the following op is a commit, xlog_recover_reorder_trans() runs
> ITEM_TYPE() on each item to look up its ops vector.  ITEM_TYPE() reads
> *(unsigned short *)item->ri_buf[0].iov_base, which faults on the
> NULL ri_buf.  The commit handlers dereference ri_buf[0] too, so reorder
> is the first place that trips over it.
> 
> This is reachable at mount time from a crafted image whose log holds the
> op sequence START_TRANS / bare-header / COMMIT_TRANS:

Given that the log is checksummed this vould be a maliciously
tweaked image and not a corruption.  It would be good to state
this thread model.

>  
> +		/* corrupt log: an item with no regions has a NULL ri_buf */
> +		if (!item->ri_cnt || !item->ri_buf) {
> +			xfs_warn(log->l_mp,
> +				"%s: committed log item has no regions",
> +				__func__);
> +			ASSERT(0);
> +			if (!list_empty(&sort_list))
> +				list_splice_init(&sort_list, &trans->r_itemq);
> +			error = -EFSCORRUPTED;
> +			break;
> +		}

That being said, handling this sounds fine, but duplicating the
code for the listing splicing feels a bit annoying.

Also I think the ASSERT(0) for on-disk corruption might be a bit
backwards these days with all the fuzzing and injecting.

So maybe as a prep-patch remove the existing ASSERT(0), then as
a second prep patch move the list splicing after the loop:

	if (error) {
		/*
		 * 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);
	} else {
		ASSERT(list_empty(&sort_list));
	}

and then add the new check.

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

[Christoph Hellwig]

On Wed, Jul 01, 2026 at 04:01:38AM -0700, Christoph Hellwig wrote:
> So maybe as a prep-patch remove the existing ASSERT(0), then as
> a second prep patch move the list splicing after the loop:
> 
> 	if (error) {
> 		/*
> 		 * 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);
> 	} else {
> 		ASSERT(list_empty(&sort_list));
> 	}

Actually just always doing the split back might be even better:

	/*
	 * Return the remaining items back to the transaction item list so they
	 * can be freed in caller.  This should only happen when we encountered
	 * an error.
	 */
	if (!list_empty(&sort_list)) {
		ASSERT(error);
		list_splice_init(&sort_list, &trans->r_itemq);
	}

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

[Weiming Shi]

Christoph Hellwig <hch@infradead.org> 于2026年7月1日周三 19:14写道：
>
> On Wed, Jul 01, 2026 at 04:01:38AM上午38点 -0700, Christoph Hellwig wrote:
> > So maybe as a prep-patch remove the existing ASSERT(0), then as
> > a second prep patch move the list splicing after the loop:
> >
> >       if (error) {
> >               /*
> >                * 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);
> >       } else {
> >               ASSERT(list_empty(&sort_list));
> >       }
>
> Actually just always doing the split back might be even better:
>
>         /*
>          * Return the remaining items back to the transaction item list so they
>          * can be freed in caller在呼叫者中.  This should only happen when we encountered遇到
>          * an error.
>          */
>         if (!list_empty(&sort_list)) {
>                 ASSERT(error);
>                 list_splice_init(&sort_list, &trans->r_itemq);
>         }


Hi Christoph,

Thanks for the review. sent v2.