Re: [PATCH v2 4/5] xfs: attr leaf verifier needs to check for obviously bad count

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

On Tue, Jan 16, 2018 at 07:50:45AM -0500, Brian Foster wrote:
> On Mon, Jan 15, 2018 at 12:05:27PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In the attribute leaf verifier, we can check for obviously bad values of
> > firstused and count so that later attempts at lasthash don't run off the
> > end of the memory buffer.  Found by ones fuzzing hdr.count in xfs/400 with
> > KASAN.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: strengthen checking, clarify what we're testing via comments
> > ---
> 
> Ok, digging more into the code I see that firstused refers to the lowest
> ->nameidx of the xfs_attr_leaf_entry's in the block. On insert,
> ->nameidx is constructed from freemap.base, which starts at just beyond
> the block header. freemap.base+freemap.size essentially refers to a raw
> block offset where the name/value info is placed, so I think I just
> misread that code the first time around. This makes sense and the
> cleanups look good. Thanks for the clarification.
> 
> One more small question..
> 
> >  fs/xfs/libxfs/xfs_attr_leaf.c |   26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 6fddce7..f1a1c60 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk(
> >  
> >  static xfs_failaddr_t
> >  xfs_attr3_leaf_verify(
> > -	struct xfs_buf		*bp)
> > +	struct xfs_buf			*bp)
> >  {
> > -	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > -	struct xfs_attr_leafblock *leaf = bp->b_addr;
> > -	struct xfs_perag *pag = bp->b_pag;
> > -	struct xfs_attr3_icleaf_hdr ichdr;
> > +	struct xfs_attr3_icleaf_hdr	ichdr;
> > +	struct xfs_mount		*mp = bp->b_target->bt_mount;
> > +	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> > +	struct xfs_perag		*pag = bp->b_pag;
> > +	struct xfs_attr_leaf_entry	*entries;
> >  
> >  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
> >  
> > @@ -282,6 +283,21 @@ xfs_attr3_leaf_verify(
> >  	if (pag && pag->pagf_init && ichdr.count == 0)
> >  		return __this_address;
> >  
> > +	/*
> > +	 * firstused is the block offset of the first name info structure.
> > +	 * Make sure it doesn't go off the block or crash into the header.
> > +	 */
> > +	if (ichdr.firstused > mp->m_attr_geo->blksize)
> > +		return __this_address;
> > +	if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf))
> > +		return __this_address;
> > +
> > +	/* Make sure the entries array doesn't crash into the name info. */
> > +	entries = xfs_attr3_leaf_entryp(bp->b_addr);
> > +	if ((char *)&entries[ichdr.count] >=
> > +	    (char *)bp->b_addr + ichdr.firstused)
> > +		return __this_address;
> > +
> 
> Can the end of the entries list align with the first namelist object if
> the block is full, for example? E.g., is '&entries[ichdr.count] ==
> bp->b_addr + ichdr.firstused' a sane possibility?

Yes.  Good catch!  I'll change the '>=' to '>'.

--D

> That aside, this looks good to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  	/* XXX: need to range check rest of attr header values */
> >  	/* XXX: hash order check? */
> >  
> > --
> > 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
> --
> 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