Re: [PATCH v2] xfs: bound logged region access in inode buffer recovery
From: Hongling Zeng
Date: Mon Sep 07 2026 - 22:19:32 EST
在 2026年09月08日 05:36, Dave Chinner 写道:
On Mon, Sep 07, 2026 at 04:04:50PM +0800, Hongling Zeng wrote:Hi Dave,
xlog_recover_do_inode_buffer() reads the logged di_next_unlinked field<sigh>
from a log record buffer at a computed offset:
logged_nextp = item->ri_buf[item_index].iov_base +
next_unlinked_offset - reg_buf_offset;
*buffer_nextp = *logged_nextp;
The only protection against reading past the log record buffer are
ASSERT()s, which compile away on non-DEBUG kernels. The existing
XFS_IS_CORRUPT(*logged_nextp == 0) check also dereferences the pointer
before validating that the computed offset lies within the logged region.
A crafted log record can make the computed offset exceed iov_len, causing
an out-of-bounds read from the log record buffer during inode buffer
recovery.
Convert the relevant ASSERT-only checks into runtime corruption checks and
verify that the logged di_next_unlinked field lies entirely within the log
iovec before dereferencing it.
Fixes: 1094d3f12363 ("xfs: refactor log recovery buffer item dispatch for pass2 commit functions")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
I'm going to say what I've said before again here, and it applies to
the BLF bitmap range checks patches you posted yesterday, too:
Hacking the same types of region size and range checks into every
log item type in an ad-hoc manner is not the right way to address
these log item verification issues.
We need to add a robust verification layer to the journal to verify
all the journal level metadata (e.g. ophdrs, transaction headers,
initial log item regions, per-log item type verification, etc) so
that we check *all* the journal items for sanity before we use them.
This is the same architecture we use for metadata (the verifier
layer) and it applies to the journal for the same reasons and
provides the same benefits (i.e. validate at first access, rest of
the code can assume validity and not have to clutter logic with
random validity checks to prevent bad behaviour.)
The high level design doc and rough plan I put together last time I
brought this up is in the patch below. If you're not willing or able
to spent time and tokens on fixing this entire class of problems for
everyone, then let please let me know ASAP.
-Dave.
Thanks for the detailed feedback. I understand the concern: the ad-hoc
region checks I posted don't address log recovery validation
systematically.
I'll withdraw the inode buffer and BLF bitmap patches and switch to
the design-driven approach. I'll post your patch 5 first (the
unchecked ri_buf[ri_cnt-1] in add_to_cont_trans(), the unbounded
kvrealloc() there, the missing oh_len alignment check in
process_data() - all reachable bugs, no restructuring), then work
through the rest of Phase 1.
Three points to confirm before coding:
1. I don't see a write-side guarantee that a region is split at most
once - xlog_write_partial() can emit multiple continuations. So
beyond your patch 2 treating the header as a normal accumulated
region, note that today a third fragment walks into
ri_buf[ri_cnt-1] with ri_cnt == 0 / ri_buf == NULL. Correct?
2. For the zero-length first fragment: if this is old-log
compatibility only, I'd document it that way and keep the special
handling as narrow as possible rather than introducing a generic
anonymous item state. Note the doc's ri_in_continuation has no
owner at that point since no item exists yet - narrow handling
avoids that entirely.
3. For testing I'll cover each phase with crafted-log images and run
xfstests. My bigger worry is over-strict validation rejecting valid
logs, which only shows up replaying real crash logs - what
coverage do you expect there? And should the design doc go in
first so the series can reference it?
Thanks,
Hongling