Re: [RFC PATCH 9/8] xfs: AGI length should be bounds checked

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 29, 2023 at 12:42:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Similar to the recent patch strengthening the AGF agf_length
> verification, the AGI verifier does not check that the AGI length field
> is within known good bounds.  This isn't currently checked by runtime
> kernel code, yet we assume in many places that it is correct and verify
> other metadata against it.
> 
> Add length verification to the AGI verifier.  Just like the AGF length
> checking, the length of the AGI must be equal to the size of the AG
> specified in the superblock, unless it is the last AG in the filesystem.
> In that case, it must be less than or equal to sb->sb_agblocks and
> greater than XFS_MIN_AG_BLOCKS, which is the smallest AG a growfs
> operation will allow to exist.
> 
> There's only one place in the filesystem that actually uses agi_length,
> but let's not leave it vulnerable to the same weird nonsense that
> generates syzbot bugs, eh?
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> NOTE: Untested, but this patch builds...
> ---
>  fs/xfs/libxfs/xfs_ialloc.c |   49 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 1e5fafbc0cdb..fec6713e1fa9 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -2486,11 +2486,12 @@ xfs_ialloc_log_agi(
>  
>  static xfs_failaddr_t
>  xfs_agi_verify(
> -	struct xfs_buf	*bp)
> +	struct xfs_buf		*bp)
>  {
> -	struct xfs_mount *mp = bp->b_mount;
> -	struct xfs_agi	*agi = bp->b_addr;
> -	int		i;
> +	struct xfs_mount	*mp = bp->b_mount;
> +	struct xfs_agi		*agi = bp->b_addr;
> +	uint32_t		agi_length = be32_to_cpu(agi->agi_length);
> +	int			i;
>  
>  	if (xfs_has_crc(mp)) {
>  		if (!uuid_equal(&agi->agi_uuid, &mp->m_sb.sb_meta_uuid))
> @@ -2507,6 +2508,37 @@ xfs_agi_verify(
>  	if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum)))
>  		return __this_address;
>  
> +	/*
> +	 * Both agi_seqno and agi_length need to validated before anything else
> +	 * block number related in the AGI can be checked.
> +	 *
> +	 * During growfs operations, the perag is not fully initialised,
> +	 * so we can't use it for any useful checking. growfs ensures we can't
> +	 * use it by using uncached buffers that don't have the perag attached
> +	 * so we can detect and avoid this problem.
> +	 */
> +	if (bp->b_pag && be32_to_cpu(agi->agi_seqno) != bp->b_pag->pag_agno)
> +		return __this_address;
> +
> +	/*
> +	 * Only the last AGI in the filesytsem is allowed to be shorter
> +	 * than the AG size recorded in the superblock.
> +	 */
> +	if (agi_length != mp->m_sb.sb_agblocks) {
> +		/*
> +		 * During growfs, the new last AGI can get here before we
> +		 * have updated the superblock. Give it a pass on the seqno
> +		 * check.
> +		 */
> +		if (bp->b_pag &&
> +		    be32_to_cpu(agi->agi_seqno) != mp->m_sb.sb_agcount - 1)
> +			return __this_address;
> +		if (agi_length < XFS_MIN_AG_BLOCKS)
> +			return __this_address;
> +		if (agi_length > mp->m_sb.sb_agblocks)
> +			return __this_address;
> +	}

I'd pull this into a helper function that both the AGF and AGI
verifiers call. It's the same checks, with the same caveats and
growfs landmines, so I think would be better as a helper...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com