Re: [PATCH] xfs: validate recovered buffer item dirty bitmaps

[Brian Foster <bfoster@redhat.com>]

On Thu, Jun 11, 2026 at 02:23:14PM -0700, Kyle Zeng wrote:
> XFS buffer log recovery trusts the dirty bitmap in recovered buffer log
> items when replaying logged regions into the buffer.  The replay path only
> has debug assertions to check that each bitmap extent fits within the
> buffer described by blf_len.
> 
> A corrupted log can therefore set a dirty bit beyond the end of the
> recovered buffer.  In non-debug builds, xlog_recover_do_reg_buffer() will
> copy the corresponding logged region to that out-of-range buffer offset.
> 
> Validate the recovered dirty bitmap and the associated logged data regions
> before replay.  Reject buffer items whose dirty extents extend beyond
> blf_len, whose region count does not match the bitmap, or whose data iovecs
> are malformed.
> 
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> ---

This looks reasonable to me at first look, but a few questions..

Do you have a test or reproducer to trigger this issue that we could
incorportate into fstests? That said, I am wondering if that might be
difficult if say checksumming gets in the way, but it can't hurt to
think about at least.

>  fs/xfs/xfs_buf_item_recover.c | 63 +++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 02b95b89d1b5..576d6245e922 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -206,6 +206,63 @@ xlog_recover_buf_commit_pass1(
>  	return 0;
>  }
>  
> +/*
> + * Make sure the recovered dirty bitmap only describes ranges inside the
> + * recovered buffer and that each logged region is backed by one data iovec.
> + */
> +STATIC int xlog_recover_validate_buf_data_map(struct xfs_mount *mp,
> +					      struct xlog_recover_item *item,
> +					      struct xfs_buf_log_format *buf_f)
> +{
> +	u64				buf_bytes = BBTOB(buf_f->blf_len);
> +	int				i = 1;
> +	int				bit = 0;
> +
> +	while (1) {
> +		u64			reg_bytes;
> +		u64			reg_end;
> +		int			nbits;
> +
> +		bit = xfs_next_bit(buf_f->blf_data_map, buf_f->blf_map_size,
> +				   bit);
> +		if (bit == -1)
> +			break;
> +
> +		nbits = xfs_contig_bits(buf_f->blf_data_map,
> +					buf_f->blf_map_size, bit);
> +		if (XFS_IS_CORRUPT(mp, nbits <= 0))
> +			return -EFSCORRUPTED;
> +
> +		reg_bytes = (u64)nbits << XFS_BLF_SHIFT;
> +		reg_end = ((u64)bit << XFS_BLF_SHIFT) + reg_bytes;
> +		if (XFS_IS_CORRUPT(mp, reg_end > buf_bytes))
> +			return -EFSCORRUPTED;
> +
> +		if (XFS_IS_CORRUPT(mp, i >= item->ri_total))
> +			return -EFSCORRUPTED;
> +		if (XFS_IS_CORRUPT(mp, !item->ri_buf[i].iov_base))
> +			return -EFSCORRUPTED;
> +		if (XFS_IS_CORRUPT(mp, item->ri_buf[i].iov_len == 0 ||
> +				   item->ri_buf[i].iov_len % XFS_BLF_CHUNK))
> +			return -EFSCORRUPTED;
> +		if (XFS_IS_CORRUPT(mp, item->ri_buf[i].iov_len > reg_bytes))
> +			return -EFSCORRUPTED;
> +

It looks like these checks are mostly promoted from asserts in
_do_reg_buffer(), right? If so that is worth pointing out in the commit
log. I'm also curious why we'd keep the asserts around at all, at least
the ones that are purely duplicated by the runtime checks, since it
seems we can no longer hit them.

> +		/*
> +		 * A single contiguous dirty range can be split into multiple
> +		 * log vectors.  Advance by the amount covered by this iovec.
> +		 */
> +		nbits = item->ri_buf[i].iov_len >> XFS_BLF_SHIFT;
> +		i++;
> +		bit += nbits;

Hmm along the same lines, I see something like this being duplicated and
wonder.. why not promote the asserts to runtime checks within the
do_reg_buffer() function where we already walk the vectors (and modify
so it can return error)?

Do we miss any protection by doing that that I'm not seeing? If not,
then at least we're not spinning through the list multiple times or
duplicating logic quirks like the above. Hm?

Brian

> +	}
> +
> +	if (XFS_IS_CORRUPT(mp, i != item->ri_total))
> +		return -EFSCORRUPTED;
> +
> +	return 0;
> +}
> +
>  /*
>   * Validate the recovered buffer is of the correct type and attach the
>   * appropriate buffer operations to them for writeback. Magic numbers are in a
> @@ -1033,6 +1090,12 @@ xlog_recover_buf_commit_pass2(
>  			goto cancelled;
>  	}
>  
> +	error = xlog_recover_validate_buf_data_map(mp, item, buf_f);
> +	if (error) {
> +		ASSERT(error == -EFSCORRUPTED);
> +		return error;
> +	}
> +
>  	trace_xfs_log_recover_buf_recover(log, buf_f);
>  	error = xfs_buf_read(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len,
>  			  0, &bp, NULL);
> -- 
> 2.43.0
>