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

[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@xxxxxx>
Signed-off-by: Guikun Yang <yangguikun19@xxxxxx>
---
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

Resend note: the first posting went out with a truncated Subject line and
without linux-kernel on Cc - a defect in the tooling that sent it, no
content changed.