Re: NULL pointer dereference in xfs_bmap_extents_to_btree() when mounting and operating a crafted image

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jun 04, 2018 at 09:03:41AM +1000, Dave Chinner wrote:
> On Mon, Jun 04, 2018 at 08:34:35AM +1000, Dave Chinner wrote:
> > On Sun, Jun 03, 2018 at 06:19:54PM -0400, Wen Xu wrote:
> > > Hi XFS developers and maintainers,
> > > Please check: https://bugzilla.kernel.org/show_bug.cgi?id=199915
> > 
> > Please report bugs straight to this list - it's much easier to
> > track and faster to discuss bugs through email than it is through
> > bugzilla.
> 
> Image log size is too small for the filesystem config. If this was
> a v5 filesystem, then the image would refuse to mount right there.
> 
> As it is, a CONFIG_XFS_DEBUG=y kernel assert fails at mount in
> xfs_check_agi_unlinked().
> 
> #ifdef DEBUG
> STATIC void
> xfs_check_agi_unlinked(
>         struct xfs_agi          *agi)
> {
>         int                     i;
> 
>         for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++)
>                 ASSERT(agi->agi_unlinked[i]);
> }
> #else
> #define xfs_check_agi_unlinked(agi)
> #endif
> 
> With the patch below, the image fails to mount with an error:
> 
> [   89.695774] XFS (loop0): Mounting V4 Filesystem
> [   89.696941] XFS (loop0): Log size 864 blocks too small, minimum size is 942 blocks
> [   89.698603] XFS (loop0): Log size out of supported range.
> [   89.699833] XFS (loop0): Continuing onwards, but if log hangs are experienced then please report this message in the bug report.
> [   89.703651] XFS (loop0): Metadata corruption detected at xfs_agi_verify+0x153/0x170, xfs_agi block 0x2
> [   89.705349] XFS (loop0): Unmount and run xfs_repair
> [   89.706179] XFS (loop0): First 128 bytes of corrupted metadata buffer:
> [   89.707317] 00000000bef77f4d: 58 41 47 49 00 00 00 01 00 00 00 00 00 00 10 00  XAGI............
> [   89.708824] 000000003cd6f10c: 00 00 00 40 00 00 00 03 00 00 00 01 00 00 00 00  ...@............
> [   89.710309] 000000002ec75c4c: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff  ................
> [   89.711749] 00000000d83fa14d: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [   89.713127] 00000000c41242dd: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [   89.714484] 000000006c41c134: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [   89.715841] 00000000b8f9ac70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [   89.717145] 000000005258d181: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00  ................
> [   89.718444] XFS (loop0): metadata I/O error in "xfs_trans_read_buf_map" at daddr 0x2 len 1 error 117
> [   89.719834] XFS (loop0): xfs_imap_lookup: xfs_ialloc_read_agi() returned error -117, agno 0
> [   89.721099] XFS (loop0): failed to read root inode
> 
> Please rework the test image so that it mounts cleanly without
> warnings or shutdowns with the patch below and then retest the POC
> code.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> xfs: verify AGI unlinked list contains valid blocks
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> The heads of tha AGI unlinked list are only scanned on debug
> kernels when the verifier runs. Change that to always scan the heads
> and validate that the inode numbers are valid.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> ---
>  fs/xfs/libxfs/xfs_ialloc.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 1ef6ff2ca002..ec5ea02b5553 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -2465,26 +2465,13 @@ xfs_ialloc_log_agi(
>  	}
>  }
>  
> -#ifdef DEBUG
> -STATIC void
> -xfs_check_agi_unlinked(
> -	struct xfs_agi		*agi)
> -{
> -	int			i;
> -
> -	for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++)
> -		ASSERT(agi->agi_unlinked[i]);
> -}
> -#else
> -#define xfs_check_agi_unlinked(agi)
> -#endif
> -
>  static xfs_failaddr_t
>  xfs_agi_verify(
>  	struct xfs_buf	*bp)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_agi	*agi = XFS_BUF_TO_AGI(bp);
> +	int		i;
>  
>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
>  		if (!uuid_equal(&agi->agi_uuid, &mp->m_sb.sb_meta_uuid))
> @@ -2520,7 +2507,13 @@ xfs_agi_verify(
>  	if (bp->b_pag && be32_to_cpu(agi->agi_seqno) != bp->b_pag->pag_agno)
>  		return __this_address;
>  
> -	xfs_check_agi_unlinked(agi);
> +	for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++) {
> +		if (agi->agi_unlinked[i] == NULLAGINO)
> +			continue;
> +		if (!xfs_verify_ino(mp, be32_to_cpu(agi->agi_unlinked[i])))
> +			return __this_address;
> +	}
> +
>  	return NULL;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html