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

["Darrick J. Wong" <djwong@kernel.org>]

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
> @@ -461,7 +461,7 @@ xlog_recover_validate_buf_type(
>   * given buffer.  The bitmap in the buf log format structure indicates
>   * where to place the logged data.
>   */
> -STATIC void
> +STATIC int
>  xlog_recover_do_reg_buffer(
>  	struct xfs_mount		*mp,
>  	struct xlog_recover_item	*item,
> @@ -489,8 +489,24 @@ xlog_recover_do_reg_buffer(
>  		ASSERT(nbits > 0);
>  		ASSERT(item->ri_buf[i].iov_base != NULL);
>  		ASSERT(item->ri_buf[i].iov_len % XFS_BLF_CHUNK == 0);
> -		ASSERT(BBTOB(bp->b_length) >=
> -		       ((uint)bit << XFS_BLF_SHIFT) + (nbits << XFS_BLF_SHIFT));
> +		/*
> +		 * The bitmap is only trustworthy to the extent that it
> +		 * describes a region that actually fits inside the buffer we
> +		 * read in based on the (attacker-controlled) blf_len.  Do not
> +		 * rely on an ASSERT() for this -- it compiles away entirely on
> +		 * non-DEBUG kernels, which is exactly where this matters, so
> +		 * validate it for real and abort recovery of this buffer rather
> +		 * than copying past the end of it.
> +		 */
> +		if (XFS_IS_CORRUPT(mp, BBTOB(bp->b_length) <
> +				((uint)bit << XFS_BLF_SHIFT) +
> +				(nbits << XFS_BLF_SHIFT))) {
> +			xfs_alert(mp,
> +	"Bad buffer log item dirty bitmap (bit %d, nbits %d) for %d-byte buffer at daddr 0x%llx.",
> +				bit, nbits, BBTOB(bp->b_length),
> +				xfs_buf_daddr(bp));
> +			return -EFSCORRUPTED;
> +		}
>  
>  		/*
>  		 * The dirty regions logged in the buffer, even though
> @@ -544,6 +560,7 @@ xlog_recover_do_reg_buffer(
>  	ASSERT(i == item->ri_total);
>  
>  	xlog_recover_validate_buf_type(mp, bp, buf_f, current_lsn);
> +	return 0;
>  }
>  
>  /*
> @@ -552,10 +569,10 @@ xlog_recover_do_reg_buffer(
>   * (ie. USR or GRP), then just toss this buffer away; don't recover it.
>   * Else, treat it as a regular buffer and do recovery.
>   *
> - * Return false if the buffer was tossed and true if we recovered the buffer to
> - * indicate to the caller if the buffer needs writing.
> + * Return 0 if the buffer was not recovered (tossed), 1 if it was recovered and
> + * needs writing, or a negative errno if recovery of the buffer failed.
>   */
> -STATIC bool
> +STATIC int
>  xlog_recover_do_dquot_buffer(
>  	struct xfs_mount		*mp,
>  	struct xlog			*log,
> @@ -564,6 +581,7 @@ xlog_recover_do_dquot_buffer(
>  	struct xfs_buf_log_format	*buf_f)
>  {
>  	uint			type;
> +	int			error;
>  
>  	trace_xfs_log_recover_buf_dquot_buf(log, buf_f);
>  
> @@ -571,7 +589,7 @@ xlog_recover_do_dquot_buffer(
>  	 * Filesystems are required to send in quota flags at mount time.
>  	 */
>  	if (!mp->m_qflags)
> -		return false;
> +		return 0;
>  
>  	type = 0;
>  	if (buf_f->blf_flags & XFS_BLF_UDQUOT_BUF)
> @@ -584,10 +602,12 @@ xlog_recover_do_dquot_buffer(
>  	 * This type of quotas was turned off, so ignore this buffer
>  	 */
>  	if (log->l_quotaoffs_flag & type)
> -		return false;
> +		return 0;
>  
> -	xlog_recover_do_reg_buffer(mp, item, bp, buf_f, NULLCOMMITLSN);
> -	return true;
> +	error = xlog_recover_do_reg_buffer(mp, item, bp, buf_f, NULLCOMMITLSN);
> +	if (error)
> +		return error;
> +	return 1;
>  }
>  
>  /*
> @@ -724,7 +744,9 @@ xlog_recover_do_primary_sb_buffer(
>  	xfs_rgnumber_t			orig_rgcount = mp->m_sb.sb_rgcount;
>  	int				error;
>  
> -	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)
> +		return error;
>  
>  	if (orig_agcount == 0) {
>  		xfs_alert(mp, "Trying to grow file system without AGs");
> @@ -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;
> +		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)