Re: [PATCH] xfs: verify inline directory data forks

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

On Wed, Mar 15, 2017 at 11:00:18AM -0400, Brian Foster wrote:
> On Wed, Mar 15, 2017 at 12:28:55AM -0700, Darrick J. Wong wrote:
> > When we're reading or writing the data fork of an inline directory,
> > check the contents to make sure we're not overflowing buffers or eating
> > garbage data.  xfs/348 corrupts an inline symlink into an inline
> > directory, triggering a buffer overflow bug.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Thanks, this looks much nicer to me. I suppose some of the checks in
> xfs_dir2_sf_getdents() could be killed off as redundant with the
> verifier in place..?

Yep.

> Just a few other comments...
> 
> >  fs/xfs/libxfs/xfs_dir2_data.c  |   73 ++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_dir2_priv.h  |    2 +
> >  fs/xfs/libxfs/xfs_inode_fork.c |   22 ++++++++++--
> >  fs/xfs/libxfs/xfs_inode_fork.h |    2 +
> >  fs/xfs/xfs_inode.c             |   12 +++++--
> >  5 files changed, 104 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> > index d478065..fb6f32d 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_data.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> > @@ -33,6 +33,79 @@
> >  #include "xfs_cksum.h"
> >  #include "xfs_log.h"
> >  
> > +/* Check the consistency of an inline directory. */
> > +int
> > +xfs_dir3_inline_check(

Hm, on second thought this ought to be xfs_dir2_sf_verify and go
in libxfs/xfs_dir2_sf.c next to _dir2_sf_check (the DEBUG function).

> > +	struct xfs_mount		*mp,
> > +	struct xfs_dinode		*dip,
> > +	int				size)
> > +{
> > +	struct xfs_dir2_sf_entry	*sfep;
> > +	struct xfs_dir2_sf_entry	*next_sfep;
> > +	struct xfs_dir2_sf_hdr		*sfp;
> > +	char				*endp;
> > +	const struct xfs_dir_ops	*dops;
> > +	xfs_ino_t			ino;
> > +	int				i;
> > +	__uint8_t			filetype;
> > +
> > +	dops = xfs_dir_get_ops(mp, NULL);
> > +
> > +	/*
> > +	 * Give up if the directory is way too short.
> > +	 */
> > +	XFS_WANT_CORRUPTED_RETURN(mp, size >
> > +			offsetof(struct xfs_dir2_sf_hdr, parent));
> > +
> > +	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_PTR(dip, XFS_DATA_FORK);
> > +	endp = (char *)sfp + size;
> > +
> > +	XFS_WANT_CORRUPTED_RETURN(mp, size >=
> > +			xfs_dir2_sf_hdr_size(sfp->i8count));
> > +
> > +	/* Check .. entry */
> > +	ino = dops->sf_get_parent_ino(sfp);
> > +	XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > +
> > +	/*
> > +	 * Loop while there are more entries and put'ing works.
> > +	 */
> 
> Probably need to fix up the comment here.

"Check all reported entries"

> > +	sfep = xfs_dir2_sf_firstentry(sfp);
> > +	for (i = 0; i < sfp->count; i++) {
> > +		/*
> > +		 * struct xfs_dir2_sf_entry has a variable length.
> > +		 * Check the fixed-offset parts of the structure are
> > +		 * within the data buffer.
> > +		 */
> > +		XFS_WANT_CORRUPTED_RETURN(mp,
> > +				((char *)sfep + sizeof(*sfep)) < endp);
> > +
> > +		/* Don't allow names with known bad length. */
> > +		XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> > +		XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> > +
> > +		/*
> > +		 * Check that the variable-length part of the structure is
> > +		 * within the data buffer.  The next entry starts after the
> > +		 * name component, so nextentry is an acceptable test.
> > +		 */
> > +		next_sfep = dops->sf_nextentry(sfp, sfep);
> > +		XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> > +
> > +		/* Check the inode number. */
> > +		ino = dops->sf_get_ino(sfp, sfep);
> > +		XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > +
> 
> It might be useful to verify the entry offsets as well (perhaps at least
> that they are ascending, for example), particularly since we have
> limited header data to go on.

Ok.

> > +		/* Check the file type. */
> > +		filetype = dops->sf_get_ftype(sfep);
> > +		XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> > +
> > +		sfep = next_sfep;
> > +	}

It also occurs to me that we probably ought to check that we actually
hit the exact end of the buffer here.

> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Check the consistency of the data block.
> >   * The input can also be a block-format directory.
> > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > index d04547f..e4f489b 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > @@ -44,6 +44,8 @@ extern int xfs_dir2_leaf_to_block(struct xfs_da_args *args,
> >  #define	xfs_dir3_data_check(dp,bp)
> >  #endif
> >  
> > +extern int xfs_dir3_inline_check(struct xfs_mount *mp, struct xfs_dinode *dip,
> > +		int size);
> >  extern int __xfs_dir3_data_check(struct xfs_inode *dp, struct xfs_buf *bp);
> >  extern int xfs_dir3_data_read(struct xfs_trans *tp, struct xfs_inode *dp,
> >  		xfs_dablk_t bno, xfs_daddr_t mapped_bno, struct xfs_buf **bpp);
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 25c1e07..2a454cf 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -33,6 +33,8 @@
> >  #include "xfs_trace.h"
> >  #include "xfs_attr_sf.h"
> >  #include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > +#include "xfs_dir2_priv.h"
> >  
> >  kmem_zone_t *xfs_ifork_zone;
> >  
> > @@ -320,6 +322,7 @@ xfs_iformat_local(
> >  	int		whichfork,
> >  	int		size)
> >  {
> > +	int		error;
> >  
> >  	/*
> >  	 * If the size is unreasonable, then something
> > @@ -336,6 +339,12 @@ xfs_iformat_local(
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > +	if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > +		error = xfs_dir3_inline_check(ip->i_mount, dip, size);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> 
> I'm wondering if we should do this after the xfs_init_local_fork() call
> and perhaps just pass the fully initialized xfs_ifork to the verifier.
> The logic being that the memcpy() to and from the disk buffer are
> effectively the "write/read" operations of the fork...
> 
> >  	xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> >  	return 0;
> >  }
> > @@ -856,7 +865,7 @@ xfs_iextents_copy(
> >   * In these cases, the format always takes precedence, because the
> >   * format indicates the current state of the fork.
> >   */
> > -void
> > +int
> >  xfs_iflush_fork(
> >  	xfs_inode_t		*ip,
> >  	xfs_dinode_t		*dip,
> > @@ -866,6 +875,7 @@ xfs_iflush_fork(
> >  	char			*cp;
> >  	xfs_ifork_t		*ifp;
> >  	xfs_mount_t		*mp;
> > +	int			error;
> >  	static const short	brootflag[2] =
> >  		{ XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> >  	static const short	dataflag[2] =
> > @@ -874,7 +884,7 @@ xfs_iflush_fork(
> >  		{ XFS_ILOG_DEXT, XFS_ILOG_AEXT };
> >  
> >  	if (!iip)
> > -		return;
> > +		return 0;
> >  	ifp = XFS_IFORK_PTR(ip, whichfork);
> >  	/*
> >  	 * This can happen if we gave up in iformat in an error path,
> > @@ -882,7 +892,7 @@ xfs_iflush_fork(
> >  	 */
> >  	if (!ifp) {
> >  		ASSERT(whichfork == XFS_ATTR_FORK);
> > -		return;
> > +		return 0;
> >  	}
> >  	cp = XFS_DFORK_PTR(dip, whichfork);
> >  	mp = ip->i_mount;
> > @@ -894,6 +904,11 @@ xfs_iflush_fork(
> >  			ASSERT(ifp->if_bytes <= XFS_IFORK_SIZE(ip, whichfork));
> >  			memcpy(cp, ifp->if_u1.if_data, ifp->if_bytes);
> >  		}
> > +		if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > +			error = xfs_dir3_inline_check(mp, dip, ifp->if_bytes);
> > +			if (error)
> > +				return error;
> > +		}
> 
> ... and thus similarly here, with the verifier before the "write" to the
> buffer and perhaps only when the data fork has been modified (and hence
> the "write" actually occurs).
> 
> Then again, it might be prudent to run it even when only the core inode
> has changed. I'm not really tied to either way I guess.

That second argument could just be a (struct xfs_dir2_sf_hdr *) in which
case we could pass XFS_DFORK_DPTR() in _iformat_local to avoid
initializing the inode fork if we know it's going to be bad; and
ifp->if_u1.if_data in _iflush_fork so that we check the contents before
writing it into the incore xfs_dinode.

Ok, that works for me.  I wonder if we need to retain _dir2_sf_check
after adding this verifier, but as it's a debug-only function full of
ASSERTs I'm not eager to remove it.

--D

> 
> Brian
> 
> >  		break;
> >  
> >  	case XFS_DINODE_FMT_EXTENTS:
> > @@ -940,6 +955,7 @@ xfs_iflush_fork(
> >  		ASSERT(0);
> >  		break;
> >  	}
> > +	return 0;
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 7fb8365..132dc59 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> >  struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
> >  
> >  int		xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> > -void		xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > +int		xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> >  				struct xfs_inode_log_item *, int);
> >  void		xfs_idestroy_fork(struct xfs_inode *, int);
> >  void		xfs_idata_realloc(struct xfs_inode *, int, int);
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 7eaf1ef..c7fe2c2 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -3475,6 +3475,7 @@ xfs_iflush_int(
> >  	struct xfs_inode_log_item *iip = ip->i_itemp;
> >  	struct xfs_dinode	*dip;
> >  	struct xfs_mount	*mp = ip->i_mount;
> > +	int			error;
> >  
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> >  	ASSERT(xfs_isiflocked(ip));
> > @@ -3557,9 +3558,14 @@ xfs_iflush_int(
> >  	if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> >  		ip->i_d.di_flushiter = 0;
> >  
> > -	xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > -	if (XFS_IFORK_Q(ip))
> > -		xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > +	error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > +	if (error)
> > +		return error;
> > +	if (XFS_IFORK_Q(ip)) {
> > +		error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > +		if (error)
> > +			return error;
> > +	}
> >  	xfs_inobp_check(mp, bp);
> >  
> >  	/*
> > --
> > 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