diff mbox series

[3/4] xfs: namecheck directory entry names before listing them

Message ID 157198050564.2873445.4054092614183428143.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: more metadata verifier tightening | expand

Commit Message

Darrick J. Wong Oct. 25, 2019, 5:15 a.m. UTC 
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(+)

Comments

Christoph Hellwig Oct. 25, 2019, 12:56 p.m. UTC | #1 
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.
Darrick J. Wong Oct. 25, 2019, 4:04 p.m. UTC | #2 
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
Brian Foster Oct. 28, 2019, 6:19 p.m. UTC | #3 
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)))
>
Darrick J. Wong Oct. 28, 2019, 6:23 p.m. UTC | #4 
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)))
> > 
>
Christoph Hellwig Oct. 29, 2019, 7:16 a.m. UTC | #5 
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.
Darrick J. Wong Oct. 29, 2019, 4:23 p.m. UTC | #6 
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
Christoph Hellwig Oct. 30, 2019, 9:32 p.m. UTC | #7 
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.
Darrick J. Wong Oct. 30, 2019, 10:18 p.m. UTC | #8 
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 mbox series

Patch

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)))