Message ID | 157198050564.2873445.4054092614183428143.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: more metadata verifier tightening | expand |
On Thu, Oct 24, 2019 at 10:15:05PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Actually call namecheck on directory entry names before we hand them > over to userspace. This looks sensible: Reviewed-by: Christoph Hellwig <hch@lst.de> Note that we can remove the check for '/' from xfs_dir2_namecheck for currentl mainline, given that verify_dirent_name in common code now has that check.
On Fri, Oct 25, 2019 at 05:56:28AM -0700, Christoph Hellwig wrote: > On Thu, Oct 24, 2019 at 10:15:05PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Actually call namecheck on directory entry names before we hand them > > over to userspace. > > This looks sensible: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Note that we can remove the check for '/' from xfs_dir2_namecheck for > currentl mainline, given that verify_dirent_name in common code now has > that check. We can't, because this is the same function that xfs_repair uses to decide if a directory entry is garbage. --D
On Thu, Oct 24, 2019 at 10:15:05PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Actually call namecheck on directory entry names before we hand them > over to userspace. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_dir2_readdir.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > > diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c > index 283df898dd9f..a8fb0a6829fd 100644 > --- a/fs/xfs/xfs_dir2_readdir.c > +++ b/fs/xfs/xfs_dir2_readdir.c ... > @@ -208,6 +214,11 @@ xfs_dir2_block_getdents( > /* > * If it didn't fit, set the final offset to here & return. > */ > + if (!xfs_dir2_namecheck(dep->name, dep->namelen)) { > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, > + dp->i_mount); > + return -EFSCORRUPTED; > + } xfs_trans_brelse(..., bp) (here and in _leaf_getdents())? Brian > if (!dir_emit(ctx, (char *)dep->name, dep->namelen, > be64_to_cpu(dep->inumber), > xfs_dir3_get_dtype(dp->i_mount, filetype))) { > @@ -456,6 +467,11 @@ xfs_dir2_leaf_getdents( > filetype = dp->d_ops->data_get_ftype(dep); > > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > + if (!xfs_dir2_namecheck(dep->name, dep->namelen)) { > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, > + dp->i_mount); > + return -EFSCORRUPTED; > + } > if (!dir_emit(ctx, (char *)dep->name, dep->namelen, > be64_to_cpu(dep->inumber), > xfs_dir3_get_dtype(dp->i_mount, filetype))) >
On Mon, Oct 28, 2019 at 02:19:15PM -0400, Brian Foster wrote: > On Thu, Oct 24, 2019 at 10:15:05PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Actually call namecheck on directory entry names before we hand them > > over to userspace. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_dir2_readdir.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c > > index 283df898dd9f..a8fb0a6829fd 100644 > > --- a/fs/xfs/xfs_dir2_readdir.c > > +++ b/fs/xfs/xfs_dir2_readdir.c > ... > > @@ -208,6 +214,11 @@ xfs_dir2_block_getdents( > > /* > > * If it didn't fit, set the final offset to here & return. > > */ > > + if (!xfs_dir2_namecheck(dep->name, dep->namelen)) { > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, > > + dp->i_mount); > > + return -EFSCORRUPTED; > > + } > > xfs_trans_brelse(..., bp) (here and in _leaf_getdents())? Will fix. --D > Brian > > > if (!dir_emit(ctx, (char *)dep->name, dep->namelen, > > be64_to_cpu(dep->inumber), > > xfs_dir3_get_dtype(dp->i_mount, filetype))) { > > @@ -456,6 +467,11 @@ xfs_dir2_leaf_getdents( > > filetype = dp->d_ops->data_get_ftype(dep); > > > > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > > + if (!xfs_dir2_namecheck(dep->name, dep->namelen)) { > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, > > + dp->i_mount); > > + return -EFSCORRUPTED; > > + } > > if (!dir_emit(ctx, (char *)dep->name, dep->namelen, > > be64_to_cpu(dep->inumber), > > xfs_dir3_get_dtype(dp->i_mount, filetype))) > > >
On Fri, Oct 25, 2019 at 09:04:48AM -0700, Darrick J. Wong wrote: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > Note that we can remove the check for '/' from xfs_dir2_namecheck for > > currentl mainline, given that verify_dirent_name in common code now has > > that check. > > We can't, because this is the same function that xfs_repair uses to > decide if a directory entry is garbage. So we'll at least need to document that for now. And maybe find a way to not do the work twice eventually in a way that doesn't break repair.
On Tue, Oct 29, 2019 at 12:16:15AM -0700, Christoph Hellwig wrote: > On Fri, Oct 25, 2019 at 09:04:48AM -0700, Darrick J. Wong wrote: > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > > > Note that we can remove the check for '/' from xfs_dir2_namecheck for > > > currentl mainline, given that verify_dirent_name in common code now has > > > that check. > > > > We can't, because this is the same function that xfs_repair uses to > > decide if a directory entry is garbage. > > So we'll at least need to document that for now. And maybe find a way > to not do the work twice eventually in a way that doesn't break repair. What if we promote EFSCORRUPTED and EFSBADCRC to the vfs (since 5 filesystems use them now); change the VFS check function to return that; and then we can just drop the xfs readdir calls to dir2_namecheck? --D
On Tue, Oct 29, 2019 at 09:23:30AM -0700, Darrick J. Wong wrote: > > So we'll at least need to document that for now. And maybe find a way > > to not do the work twice eventually in a way that doesn't break repair. > > What if we promote EFSCORRUPTED and EFSBADCRC to the vfs (since 5 > filesystems use them now); change the VFS check function to return that; > and then we can just drop the xfs readdir calls to dir2_namecheck? EFSCORRUPTED should have moved to common code a long time ago, so that is overdue. Not sure about EFSBADCRC, but that might not be a horrible idea either.
On Wed, Oct 30, 2019 at 02:32:02PM -0700, Christoph Hellwig wrote: > On Tue, Oct 29, 2019 at 09:23:30AM -0700, Darrick J. Wong wrote: > > > So we'll at least need to document that for now. And maybe find a way > > > to not do the work twice eventually in a way that doesn't break repair. > > > > What if we promote EFSCORRUPTED and EFSBADCRC to the vfs (since 5 > > filesystems use them now); change the VFS check function to return that; > > and then we can just drop the xfs readdir calls to dir2_namecheck? Having looked more carefully at verify_dirent_name(), I now think XFS shouldn't drop the xfs_readdir calls to dir2_namecheck because the VFS namecheck function permits nulls in the middle of the name. Linus says the function does that intentionally because (in his opinion) userspace expects a null terminated string and won't care if namelen is longer than that. > EFSCORRUPTED should have moved to common code a long time ago, so that > is overdue. Not sure about EFSBADCRC, but that might not be a horrible > idea either. We might as well, since ext4 and XFS both standardized on both of those error codes. Hm, btrfs uses EUCLEAN and EIO for EFSCORRUPTED and EFSBADCRC, respectively. We probably ought to get them on board too. --D
diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c index 283df898dd9f..a8fb0a6829fd 100644 --- a/fs/xfs/xfs_dir2_readdir.c +++ b/fs/xfs/xfs_dir2_readdir.c @@ -17,6 +17,7 @@ #include "xfs_trace.h" #include "xfs_bmap.h" #include "xfs_trans.h" +#include "xfs_error.h" /* * Directory file type support functions @@ -115,6 +116,11 @@ xfs_dir2_sf_getdents( ino = dp->d_ops->sf_get_ino(sfp, sfep); filetype = dp->d_ops->sf_get_ftype(sfep); ctx->pos = off & 0x7fffffff; + if (!xfs_dir2_namecheck(sfep->name, sfep->namelen)) { + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, + dp->i_mount); + return -EFSCORRUPTED; + } if (!dir_emit(ctx, (char *)sfep->name, sfep->namelen, ino, xfs_dir3_get_dtype(dp->i_mount, filetype))) return 0; @@ -208,6 +214,11 @@ xfs_dir2_block_getdents( /* * If it didn't fit, set the final offset to here & return. */ + if (!xfs_dir2_namecheck(dep->name, dep->namelen)) { + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, + dp->i_mount); + return -EFSCORRUPTED; + } if (!dir_emit(ctx, (char *)dep->name, dep->namelen, be64_to_cpu(dep->inumber), xfs_dir3_get_dtype(dp->i_mount, filetype))) { @@ -456,6 +467,11 @@ xfs_dir2_leaf_getdents( filetype = dp->d_ops->data_get_ftype(dep); ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; + if (!xfs_dir2_namecheck(dep->name, dep->namelen)) { + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, + dp->i_mount); + return -EFSCORRUPTED; + } if (!dir_emit(ctx, (char *)dep->name, dep->namelen, be64_to_cpu(dep->inumber), xfs_dir3_get_dtype(dp->i_mount, filetype)))