Re: [PATCH] xfs: bound inode buffer log item replay by logged regions

On Wed, Jul 29, 2026 at 12:31:27PM -0700, Weiming Shi wrote:
> xlog_recover_do_inode_buffer() advances item_index once for every run of
> set bits in the logged dirty bitmap, but ri_buf[] is populated only for
> the regions present in the recovered log item.  A crafted inode buffer
> log item can therefore make the dirty bitmap describe more runs than the
> item has logged regions, and recovery reads past the end of ri_buf[]
> before dereferencing the bogus iov_base pointer.
> 
> The ASSERT() that checks iov_base compiles away without CONFIG_XFS_DEBUG,
> and evaluating it would already perform the out-of-bounds ri_buf[] read.
> Check item_index against ri_cnt before the first ri_buf[item_index]
> access instead, and fail recovery with -EFSCORRUPTED.
> 
> Found with KASAN on a CONFIG_XFS_DEBUG=n build by mounting a crafted XFS
> image whose log contains an inode buffer item with more dirty bitmap runs
> than logged regions:
> 
>  BUG: KASAN: slab-out-of-bounds in xlog_recover_do_inode_buffer (fs/xfs/xfs_buf_item_recover.c:701)
>  Read of size 8 at addr ffff88801387d160 by task mount
>  The buggy address is located 0 bytes to the right of
>   allocated 32-byte region [ffff88801387d140, ffff88801387d160)
>   xlog_recover_do_inode_buffer (fs/xfs/xfs_buf_item_recover.c:701)
>   xlog_recover_buf_commit_pass2 (fs/xfs/xfs_buf_item_recover.c:1130)
>   ...
>  Oops: general protection fault, probably for non-canonical address
>  RIP: 0010:xlog_recover_do_inode_buffer (fs/xfs/xfs_buf_item_recover.c:703)
> 
> After this change the crafted image fails recovery with -EFSCORRUPTED and
> the mount is refused.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Kyle Zeng <[email protected]>
> Link: https://lore.kernel.org/linux-xfs/[email protected]/
> Reported-by: Xiang Mei <[email protected]>
> Suggested-by: Brian Foster <[email protected]>
> Link: https://lore.kernel.org/linux-xfs/ajGB9o5Ys6itU3Rh@bfoster/
> Cc: [email protected]
> Signed-off-by: Weiming Shi <[email protected]>
> ---
>  fs/xfs/xfs_buf_item_recover.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 02b95b89d1b5..2c2d4a0027af 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -659,6 +659,13 @@ xlog_recover_do_inode_buffer(
>  			reg_buf_offset = bit << XFS_BLF_SHIFT;
>  			reg_buf_bytes = nbits << XFS_BLF_SHIFT;
>  			item_index++;
> +
> +			if (XFS_IS_CORRUPT(mp, item_index >= item->ri_cnt)) {
> +				xfs_alert(mp,
> +	"Bad inode buffer log item dirty bitmap: more dirty regions than the %d logged, for buffer at daddr 0x%llx.",
> +					item->ri_cnt - 1, xfs_buf_daddr(bp));
> +				return -EFSCORRUPTED;
> +			}
>  		}

Why are you trying to validate this here instead of via generic log
item validation infrastructure? I thought I already asked for this
sort of thing to be lifted to the initial pass of the log items in
the generic recovery code via per-item type validation methods
instead of being randomly distributed through the recovery code.

Cheers,

Dave.
-- 
Dave Chinner
[email protected]