Re: [PATCH] xfs: AGF length has never been bounds checked

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 15, 2023 at 09:19:01PM -0700, Darrick J. Wong wrote:
> On Fri, Jun 16, 2023 at 11:59:06AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The AGF verifier does not check that the AGF length field is within
> > known good bounds. This has never been checked by runtime kernel
> > code (i.e. the lack of verification goes back to 1993) yet we assume
> 
> Woo hoo!
> 
> > in many places that it is correct and verify other metdata against
> > it.
> > 
> > Add length verification to the AGF verifier. The length of the AGF
> > 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.
> > 
> > This requires a bit of rework of the verifier function. We want to
> > verify metadata before we use it to verify other metadata. Hence
> > we need to verify the AGF sequence numbers before using them to
> > verify the length of the AGF. Then we can verify the AGF length
> > before we verify AGFL fields. Then we can verifier other fields that
> > are bounds limited by the AGF length.
> > 
> > And, finally, by calculating agf_length only once into a local
> > variable, we can collapse repeated "if (xfs_has_foo() &&"
> > conditionaly checks into single checks. This makes the code much
> > easier to follow as all the checks for a given feature are obviously
> > in the same place.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c | 81 ++++++++++++++++++++++-----------------
> >  1 file changed, 46 insertions(+), 35 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 7c675aae0a0f..78556cad57e5 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2970,6 +2970,7 @@ xfs_agf_verify(
> >  {
> >  	struct xfs_mount	*mp = bp->b_mount;
> >  	struct xfs_agf		*agf = bp->b_addr;
> > +	uint32_t		agf_length = be32_to_cpu(agf->agf_length);
> >  
> >  	if (xfs_has_crc(mp)) {
> >  		if (!uuid_equal(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid))
> > @@ -2981,18 +2982,38 @@ xfs_agf_verify(
> >  	if (!xfs_verify_magic(bp, agf->agf_magicnum))
> >  		return __this_address;
> >  
> > -	if (!(XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
> > -	      be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) &&
> > -	      be32_to_cpu(agf->agf_flfirst) < xfs_agfl_size(mp) &&
> > -	      be32_to_cpu(agf->agf_fllast) < xfs_agfl_size(mp) &&
> > -	      be32_to_cpu(agf->agf_flcount) <= xfs_agfl_size(mp)))
> > +	if (!(XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum))))
> >  		return __this_address;
> >  
> > -	if (be32_to_cpu(agf->agf_length) > mp->m_sb.sb_dblocks)
> > +	/*
> > +	 * 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.
> 
> Would you mind adding an extra sentence here:
> 
> "Both agf_seqno and agf_length need to be validated before anything else
> fsblock related in the AGF."

Yup.

> > +	 */
> > +	if (bp->b_pag && be32_to_cpu(agf->agf_seqno) != bp->b_pag->pag_agno)
> > +		return __this_address;
> > +
> > +	/*
> > +	 * Only the last AGF in the filesytsem is allowed to be shorter
> > +	 * than the AG size recorded in the superblock.
> > +	 */
> > +	if (agf_length != mp->m_sb.sb_agblocks) {
> > +		if (be32_to_cpu(agf->agf_seqno) != mp->m_sb.sb_agcount - 1)
> > +			return __this_address;
> > +		if (agf_length < XFS_MIN_AG_BLOCKS)
> 
> The superblock verifier checks that sb_agblocks >= XFS_MIN_AG_BYTES,
> which means that it can't be less than 16MB.  That's the lower bound on
> the general AG size, not the lower bound of a runt AG at the end of the
> fs.

*nod*

> OTOH, the lower bound of a runt AG is XFS_MIN_AG_BLOCKS, or 64FSB.  I
> would sorta like this to be outside this sub-block since that's
> independent of whatever sb_agblocks is.
>
> That said, there is no filesystem where setting sb_agblocks to 16MB
> would result in an sb_agblocks with a value less than 256, so I suppose
> this is a moot worry of mine.
> 
> Does that make sense?

*nod*.

The sb verifier is checking valid sb_agblocks bounds, and this is
just checking the invariant that all AGs must be the same size as
sb_agblocks, except for the runt AG. The runt AG has bounds of
XFS_MIN_AG_BLOCKS <= agf_length <= sb_agblocks, so we check those
here...

> > +			return __this_address;
> > +		if (agf_length > mp->m_sb.sb_agblocks)
> > +			return __this_address;
> > +	}
> > +
> > +	if (be32_to_cpu(agf->agf_flfirst) >= xfs_agfl_size(mp) ||
> > +	    be32_to_cpu(agf->agf_fllast) >= xfs_agfl_size(mp) ||
> > +	    be32_to_cpu(agf->agf_flcount) > xfs_agfl_size(mp))
> >  		return __this_address;
> 
> I wish each check would get its own return __this_address.  Today I was
> debugging some dumb bug but addr2line dropped me off in the middle of
> this mound of code. :(

I've got to revise it for the comment above, so I can do that
easily enough here too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com