Re: [PATCH] xfs: reject a continuation op with no buffered region in log

Re: [PATCH] xfs: reject a continuation op with no buffered region in log

[Dave Chinner]

On Tue, Nov 10, 2026 at 08:00:00PM +0800, Guikun Yang wrote:
> xlog_recover_add_to_cont_trans() takes the tail item of the transaction
> and indexes ri_buf[ri_cnt - 1] without checking that the item has any
> region buffered:
> 
> 	item = list_entry(trans->r_itemq.prev, struct xlog_recover_item,
> 			  ri_list);
> 	old_ptr = item->ri_buf[item->ri_cnt-1].iov_base;
> 
> An item created by xlog_recover_add_item() has ri_cnt == 0 and
> ri_buf == NULL - the transaction header itself is copied into
> trans->r_theader, not into the item - so this evaluates to NULL[-1].
> xlog_recovery_process_trans() dispatches to this function for any op
> whose header carries XLOG_WAS_CONT_TRANS, and nothing on the way
> checks that flag sequence against the item state; the only related
> assertion in the recovery code is XFS_DEBUG-only.
> 
> A crafted dirty log with a valid transaction header followed by an op
> header with XLOG_WAS_CONT_TRANS therefore faults at mount time on a
> stock kernel, without any sanitizer enabled:
> 
>   BUG: unable to handle page fault for address: fffffffffffffff0
>   #PF: supervisor read access in kernel mode
>   Oops: 0000 [#1] SMP
>   RIP: xlog_recover_add_to_cont_trans+0xf9/0x460
>    xlog_recovery_process_trans
>    xlog_recover_process_ophdr
>    xlog_recover_process_data
>    xlog_recover_process
>    xlog_do_recovery_pass
> 
> Note that reaching it does not require an actual split region: a single
> continuation op right after the transaction header is enough, because
> the dispatcher never checks that the previous region had a buffer
> attached.
> 
> Treat it as log corruption and fail recovery, matching the other
> -EFSCORRUPTED paths in this file.
> 
> Reported-by: Guikun Yang <yangguikun19@qq.com>
> Signed-off-by: Guikun Yang <yangguikun19@qq.com>

NACK, again, for the same reasons.

-- 
Dave Chinner
dgc@kernel.org

[PATCH] xfs: reject a continuation op with no buffered region in log

[Guikun Yang]

xlog_recover_add_to_cont_trans() takes the tail item of the transaction
and indexes ri_buf[ri_cnt - 1] without checking that the item has any
region buffered:

	item = list_entry(trans->r_itemq.prev, struct xlog_recover_item,
			  ri_list);
	old_ptr = item->ri_buf[item->ri_cnt-1].iov_base;

An item created by xlog_recover_add_item() has ri_cnt == 0 and
ri_buf == NULL - the transaction header itself is copied into
trans->r_theader, not into the item - so this evaluates to NULL[-1].
xlog_recovery_process_trans() dispatches to this function for any op
whose header carries XLOG_WAS_CONT_TRANS, and nothing on the way
checks that flag sequence against the item state; the only related
assertion in the recovery code is XFS_DEBUG-only.

A crafted dirty log with a valid transaction header followed by an op
header with XLOG_WAS_CONT_TRANS therefore faults at mount time on a
stock kernel, without any sanitizer enabled:

  BUG: unable to handle page fault for address: fffffffffffffff0
  #PF: supervisor read access in kernel mode
  Oops: 0000 [#1] SMP
  RIP: xlog_recover_add_to_cont_trans+0xf9/0x460
   xlog_recovery_process_trans
   xlog_recover_process_ophdr
   xlog_recover_process_data
   xlog_recover_process
   xlog_do_recovery_pass

Note that reaching it does not require an actual split region: a single
continuation op right after the transaction header is enough, because
the dispatcher never checks that the previous region had a buffer
attached.

Treat it as log corruption and fail recovery, matching the other
-EFSCORRUPTED paths in this file.

Reported-by: Guikun Yang <yangguikun19@qq.com>
Signed-off-by: Guikun Yang <yangguikun19@qq.com>
---
Tested on a crafted dirty-log image (real mkfs.xfs, files, sync, crash,
then the first region op after the transaction header gets oh_flags
0x08 and the v2 record CRC is recomputed): the modified image now fails
recovery with the new message, while the unmodified image still
recovers and mounts. The same image reproduced the NULL deref 3/3
before the patch.

 fs/xfs/xfs_log_recover.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 1111111..2222222 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2147,6 +2147,11 @@ xlog_recover_add_to_cont_trans(
 	/* take the tail entry */
 	item = list_entry(trans->r_itemq.prev, struct xlog_recover_item,
 			  ri_list);
+	if (XFS_IS_CORRUPT(log->l_mp, item->ri_cnt == 0)) {
+		xfs_warn(log->l_mp,
+			 "%s: continuation with no buffered region", __func__);
+		return -EFSCORRUPTED;
+	}
 
 	old_ptr = item->ri_buf[item->ri_cnt-1].iov_base;
 	old_len = item->ri_buf[item->ri_cnt-1].iov_len;
--
2.43.0