On Wed, Mar 15, 2017 at 03:39:44PM -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@xxxxxxxxxx>
> ---
> v2: add more checks consistent with _dir2_sf_check and make the verifier
> usable from anywhere.
> ---
> fs/xfs/libxfs/xfs_dir2_priv.h | 2 +
> fs/xfs/libxfs/xfs_dir2_sf.c | 85 ++++++++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_inode_fork.c | 26 +++++++++++-
> fs/xfs/libxfs/xfs_inode_fork.h | 2 -
> fs/xfs/xfs_dir2_readdir.c | 11 -----
> fs/xfs/xfs_inode.c | 12 ++++--
> 6 files changed, 120 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> index d04547f..eb00bc1 100644
> --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> @@ -125,6 +125,8 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> +extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> + int size);
>
> /* xfs_dir2_readdir.c */
> extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index c6809ff..b0cfc7d 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -629,6 +629,91 @@ xfs_dir2_sf_check(
> }
> #endif /* DEBUG */
>
> +/* Verify the consistency of an inline directory. */
> +int
> +xfs_dir2_sf_verify(
> + struct xfs_mount *mp,
> + struct xfs_dir2_sf_hdr *sfp,
> + int size)
> +{
> + struct xfs_dir2_sf_entry *sfep;
> + struct xfs_dir2_sf_entry *next_sfep;
> + char *endp;
> + const struct xfs_dir_ops *dops;
> + xfs_ino_t ino;
> + int i;
> + int i8count;
> + int offset;
> + __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));
> + XFS_WANT_CORRUPTED_RETURN(mp, size >=
> + xfs_dir2_sf_hdr_size(sfp->i8count));
> +
> + endp = (char *)sfp + size;
> +
> + /* Check .. entry */
> + ino = dops->sf_get_parent_ino(sfp);
> + i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> + XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> + offset = dops->data_first_offset;
> +
> + /* 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 that the offsets always increase. */
> + XFS_WANT_CORRUPTED_RETURN(mp,
> + xfs_dir2_sf_get_offset(sfep) >= offset);
> +
> + /* Check the inode number. */
> + ino = dops->sf_get_ino(sfp, sfep);
> + i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> + XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> +
> + /* Check the file type. */
> + filetype = dops->sf_get_ftype(sfep);
> + XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> +
> + offset = xfs_dir2_sf_get_offset(sfep) +
> + dops->data_entsize(sfep->namelen);
> +
> + sfep = next_sfep;
> + }
> + XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> + XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> + XFS_WANT_CORRUPTED_RETURN(mp, offset +
> + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> + (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
Is this to check whether the dir should be in local format (if so,
obscure enough to warrant a comment imo)?
> +
> + return 0;
> +}
> +
> /*
> * Create a new (shortform) directory.
> */
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 25c1e07..9653e96 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,14 @@ xfs_iformat_local(
> return -EFSCORRUPTED;
> }
>
> + if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> + error = xfs_dir2_sf_verify(ip->i_mount,
> + (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> + size);
> + if (error)
> + return error;
> + }
> +
I was thinking more to verify the in-core fork after it is initialized,
but before it is used (i.e., the memcpy() being analogous to I/O with
respect to buffer verifiers). This works too though, so long as the size
checks and whatnot in the verifier remain valid against the inode size
as they do against the data fork size. Either way:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> return 0;
> }
> @@ -856,7 +867,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 +877,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 +886,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,12 +894,19 @@ xfs_iflush_fork(
> */
> if (!ifp) {
> ASSERT(whichfork == XFS_ATTR_FORK);
> - return;
> + return 0;
> }
> cp = XFS_DFORK_PTR(dip, whichfork);
> mp = ip->i_mount;
> switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> case XFS_DINODE_FMT_LOCAL:
> + if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> + error = xfs_dir2_sf_verify(mp,
> + (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> + ifp->if_bytes);
> + if (error)
> + return error;
> + }
> if ((iip->ili_fields & dataflag[whichfork]) &&
> (ifp->if_bytes > 0)) {
> ASSERT(ifp->if_u1.if_data != NULL);
> @@ -940,6 +959,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_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index 003a99b..ad9396e 100644
> --- a/fs/xfs/xfs_dir2_readdir.c
> +++ b/fs/xfs/xfs_dir2_readdir.c
> @@ -71,22 +71,11 @@ xfs_dir2_sf_getdents(
> struct xfs_da_geometry *geo = args->geo;
>
> ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
> - /*
> - * Give up if the directory is way too short.
> - */
> - if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
> - ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
> - return -EIO;
> - }
> -
> ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
> ASSERT(dp->i_df.if_u1.if_data != NULL);
>
> sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
>
> - if (dp->i_d.di_size < xfs_dir2_sf_hdr_size(sfp->i8count))
> - return -EFSCORRUPTED;
> -
> /*
> * If the block number in the offset is out of range, we're done.
> */
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
