| Message ID | 20170315072855.GA5280@birch.djwong.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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..? 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( > + 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. > + 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. > + /* Check the file type. */ > + filetype = dops->sf_get_ftype(sfep); > + XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX); > + > + sfep = next_sfep; > + } > + > + 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. 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
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 -- 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
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. I think the check is fine, but from a structural point of view they are in the wrong place. i.e. the functions xfs_iformat_local() and xfs_iflush_fork() should not be doing any content specific checks and verification. All they do is marshall the fork data to and from in-memory and on-disk formats - the contents of the forks should be opaque to them. IOWs, incoming fork contents validity should be checked in xfs_iformat_fork() after we call xfs_iformat_local(), outgoing fork validity is checked in xfs_iflush_int() before calling xfs_iflush_fork(). Cheers, Dave.
On Fri, Mar 17, 2017 at 08:12:56AM +1100, Dave Chinner 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. > > I think the check is fine, but from a structural point of view they > are in the wrong place. i.e. the functions xfs_iformat_local() and > xfs_iflush_fork() should not be doing any content specific checks > and verification. All they do is marshall the fork data to and from > in-memory and on-disk formats - the contents of the forks should be > opaque to them. > > IOWs, incoming fork contents validity should be checked in > xfs_iformat_fork() after we call xfs_iformat_local(), outgoing fork > validity is checked in xfs_iflush_int() before calling > xfs_iflush_fork(). Sorry for pushing the button prematurely. I just sent out a patch to clean this up and address a few other issues. --D > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- 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
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( + 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. + */ + 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)); + + /* Check the file type. */ + filetype = dops->sf_get_ftype(sfep); + XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX); + + sfep = next_sfep; + } + + 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; + } + 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; + } 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); /*
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> --- 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(-) -- 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