Re: [PATCH v4] xfs: bounds-check buffer log item's dirty bitmap

[Brian Foster <bfoster@redhat.com>]

On Tue, Jul 14, 2026 at 11:01:52AM -0700, Darrick J. Wong wrote:
> On Tue, Jul 14, 2026 at 07:55:32PM +0200, Ibrahim Hashimov wrote:
> > xlog_recover_do_reg_buffer() replays each dirty region described by a
> > buffer log item's bitmap into the buffer read for that item:
> > 
> > 	memcpy(xfs_buf_offset(bp, (uint)bit << XFS_BLF_SHIFT),
> > 		item->ri_buf[i].iov_base,
> > 		nbits << XFS_BLF_SHIFT);
> > 
> > The destination offset (bit/nbits, from the logged dirty bitmap) and the
> > buffer size (from the logged blf_len) are both attacker-controlled and
> > otherwise unrelated, yet the only thing bounding the copy is an ASSERT(),
> > which compiles away on production kernels. A crafted image logging a
> > small blf_len together with a bitmap bit past the end of that buffer
> > drives the memcpy() past the buffer's allocation, corrupting adjacent
> > kernel heap during mount-time log recovery. This is reachable by anyone
> > who can get a crafted image mounted -- the malicious-filesystem threat
> > model XFS already guards against elsewhere.
> > 
> > Turn the ASSERT() into a real XFS_IS_CORRUPT() check that aborts recovery
> > of the buffer with -EFSCORRUPTED, consistent with the validate-and-fail
> > idiom already used in xlog_recover_do_inode_buffer() and
> > xfs_dquot_item_recover.c. xlog_recover_do_reg_buffer() therefore becomes
> > STATIC int and its three callers propagate the error.
> > 
> > Found and confirmed with KASAN on a CONFIG_XFS_DEBUG=n build: the crafted
> > image trips a slab-out-of-bounds write before this change and fails
> > recovery cleanly with -EFSCORRUPTED after it.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
> > Assisted-by: AuditCode-AI:2026.07
> 
> Looks fine to me now, thanks for making those edits.
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 
> --D
> 
> > ---
> > v4: fold xlog_recover_do_dquot_buffer()'s bool return and error
> >     out-parameter into a single int return (1 if dirty, 0 if clean, or a
> >     negative errno on failure), per Darrick's review. No behavioural
> >     change.
> > v3: trim the changelog per Brian Foster's review. Add a Fixes: tag --
> >     the destination-bounds check has been an ASSERT since the initial git
> >     import (2.6.12-rc2), so it predates the git era.
> > v2: resend; v1 went out with an empty Subject line due to a local
> >     git send-email glitch (leading blank line in the patch file).
> > 
> >  fs/xfs/xfs_buf_item_recover.c | 56 ++++++++++++++++++++++++++++-------------
> >  1 file changed, 40 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> > index 02b95b89d1b5..cf2b07ebc6f3 100644
> > --- a/fs/xfs/xfs_buf_item_recover.c
> > +++ b/fs/xfs/xfs_buf_item_recover.c
...
> > @@ -1081,11 +1103,10 @@ xlog_recover_buf_commit_pass2(
> >  			goto out_release;
> >  	} else if (buf_f->blf_flags &
> >  		  (XFS_BLF_UDQUOT_BUF|XFS_BLF_PDQUOT_BUF|XFS_BLF_GDQUOT_BUF)) {
> > -		bool	dirty;
> > -
> > -		dirty = xlog_recover_do_dquot_buffer(mp, log, item, bp, buf_f);
> > -		if (!dirty)
> > +		error = xlog_recover_do_dquot_buffer(mp, log, item, bp, buf_f);
> > +		if (error <= 0)
> >  			goto out_release;

I might suggest something like:

		/* reset error since > 0 means to write the buffer */

... or maybe we can phrase that better. But regardless LGTM now as well,
thanks:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> > +		error = 0;
> >  	} else if ((xfs_blft_from_flags(buf_f) & XFS_BLFT_SB_BUF) &&
> >  			xfs_buf_daddr(bp) == 0) {
> >  		error = xlog_recover_do_primary_sb_buffer(mp, item, bp, buf_f,
> > @@ -1105,7 +1126,10 @@ xlog_recover_buf_commit_pass2(
> >  			xfs_buf_relse(rtsb_bp);
> >  		}
> >  	} else {
> > -		xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn);
> > +		error = xlog_recover_do_reg_buffer(mp, item, bp, buf_f,
> > +						   current_lsn);
> > +		if (error)
> > +			goto out_release;
> >  	}
> >  
> >  	/*
> > -- 
> > 2.50.1 (Apple Git-155)
>