Re: [PATCH v2 RESEND] xfs: bound inode fork length against the fork size during log recovery

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

On Wed, Sep 23, 2026 at 08:57:44AM -0300, Aldo Ariel Panzardo wrote:
> xlog_recover_inode_commit_pass2() copies an inode log item's data/attr
> fork region into the inode buffer using the on-log region length without
> bounding it against the fork capacity, e.g.:
> 
> 	len = item->ri_buf[2].iov_len;
> 	memcpy(XFS_DFORK_DPTR(dip), src, len);
> 
> The only guard is an ASSERT, which is a no-op on production kernels
> (CONFIG_XFS_DEBUG off), and xfs_dinode_verify() runs only after the copy.
> A crafted image with a dirty log can therefore drive a heap out-of-bounds
> write at mount time. The XFS_ILOG_DBROOT sibling already passes
> XFS_DFORK_DSIZE as a bound; the DDATA/DEXT and ADATA/AEXT memcpy paths
> did not.
> 
> Bound each logged fork region against the destination fork size before
> copying it, and reject the log item with -EFSCORRUPTED when it does not
> fit. Because the recovered inode is only verified after the fork data has
> been copied in, the checks are done up front, before any memcpy into the
> on-disk inode.
> 
> Fixes: 658fa68b6f34 ("xfs: refactor log recovery inode item dispatch for pass2 commit functions")
> Cc: <stable@vger.kernel.org> # v5.8
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

Looks correct to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
> v2: cc stable # v5.8 (per Darrick).  Move both fork-length checks to the
>     top of the fork-copy block, before any memcpy into the on-disk
>     inode, and drop the now-redundant ASSERT.
> 
>  fs/xfs/xfs_inode_item_recover.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> index 169a8fe3bf0a..6c7dd7dd7032 100644
> --- a/fs/xfs/xfs_inode_item_recover.c
> +++ b/fs/xfs/xfs_inode_item_recover.c
> @@ -507,6 +507,25 @@ xlog_recover_inode_commit_pass2(
>  	ASSERT(!(fields & XFS_ILOG_DFORK) ||
>  	       (len == xlog_calc_iovec_len(in_f->ilf_dsize)));
>  
> +	/*
> +	 * The recovered inode is verified only after the fork data has been
> +	 * copied into it, so bound each logged fork region against the size of
> +	 * its fork now, before the memcpy below can overrun the on-disk inode.
> +	 * The DBROOT/ABROOT cases already bound their copies against the fork
> +	 * size.
> +	 */
> +	if ((fields & (XFS_ILOG_DDATA | XFS_ILOG_DEXT)) &&
> +	    item->ri_buf[2].iov_len > XFS_DFORK_DSIZE(dip, mp)) {
> +		error = -EFSCORRUPTED;
> +		goto out_release;
> +	}
> +	if ((fields & (XFS_ILOG_ADATA | XFS_ILOG_AEXT)) &&
> +	    item->ri_buf[(fields & XFS_ILOG_DFORK) ? 3 : 2].iov_len >
> +	    XFS_DFORK_ASIZE(dip, mp)) {
> +		error = -EFSCORRUPTED;
> +		goto out_release;
> +	}
> +
>  	switch (fields & XFS_ILOG_DFORK) {
>  	case XFS_ILOG_DDATA:
>  	case XFS_ILOG_DEXT:
> @@ -546,7 +565,6 @@ xlog_recover_inode_commit_pass2(
>  		case XFS_ILOG_ADATA:
>  		case XFS_ILOG_AEXT:
>  			dest = XFS_DFORK_APTR(dip);
> -			ASSERT(len <= XFS_DFORK_ASIZE(dip, mp));
>  			memcpy(dest, src, len);
>  			break;
>  
> -- 
> 2.53.0
> 
>