[PATCH 0/4] xfs: more metadata verifier tightening

[PATCH 0/4] xfs: more metadata verifier tightening

[Darrick J. Wong]

Hi all,

Here are some enhancements I made to the metadata verifiers.  The first
adds structure checking to the attr leaf verifier.  The next two look
for obviously invalid dirent and attr names before passing them up to
the VFS.  The fourth patch fixes some problems where we return EIO on
metadata corruption instead of EFSCORRUPTED.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=tighten-verifiers

[PATCH 1/4] xfs: check attribute leaf block structure

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add missing structure checks in the attribute leaf verifier.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c |   63 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index ec7921e07f69..8dea3a273029 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -232,6 +232,57 @@ xfs_attr3_leaf_hdr_to_disk(
 	}
 }
 
+static xfs_failaddr_t
+xfs_attr3_leaf_verify_entry(
+	struct xfs_mount			*mp,
+	char					*buf_end,
+	struct xfs_attr_leafblock		*leaf,
+	struct xfs_attr3_icleaf_hdr		*leafhdr,
+	struct xfs_attr_leaf_entry		*ent,
+	int					idx,
+	__u32					*last_hashval)
+{
+	struct xfs_attr_leaf_name_local		*lentry;
+	struct xfs_attr_leaf_name_remote	*rentry;
+	char					*name_end;
+	unsigned int				nameidx;
+	unsigned int				namesize;
+	__u32					hashval;
+
+	/* hash order check */
+	hashval = be32_to_cpu(ent->hashval);
+	if (hashval < *last_hashval)
+		return __this_address;
+	*last_hashval = hashval;
+
+	nameidx = be16_to_cpu(ent->nameidx);
+	if (nameidx < leafhdr->firstused || nameidx >= mp->m_attr_geo->blksize)
+		return __this_address;
+
+	/* Check the name information. */
+	if (ent->flags & XFS_ATTR_LOCAL) {
+		lentry = xfs_attr3_leaf_name_local(leaf, idx);
+		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
+				be16_to_cpu(lentry->valuelen));
+		name_end = (char *)lentry + namesize;
+		if (lentry->namelen == 0)
+			return __this_address;
+	} else {
+		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
+		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
+		name_end = (char *)rentry + namesize;
+		if (rentry->namelen == 0)
+			return __this_address;
+		if (rentry->valueblk == 0)
+			return __this_address;
+	}
+
+	if (name_end > buf_end)
+		return __this_address;
+
+	return NULL;
+}
+
 static xfs_failaddr_t
 xfs_attr3_leaf_verify(
 	struct xfs_buf			*bp)
@@ -240,7 +291,10 @@ xfs_attr3_leaf_verify(
 	struct xfs_mount		*mp = bp->b_mount;
 	struct xfs_attr_leafblock	*leaf = bp->b_addr;
 	struct xfs_attr_leaf_entry	*entries;
+	struct xfs_attr_leaf_entry	*ent;
+	char				*buf_end;
 	uint32_t			end;	/* must be 32bit - see below */
+	__u32				last_hashval = 0;
 	int				i;
 	xfs_failaddr_t			fa;
 
@@ -273,8 +327,13 @@ xfs_attr3_leaf_verify(
 	    (char *)bp->b_addr + ichdr.firstused)
 		return __this_address;
 
-	/* XXX: need to range check rest of attr header values */
-	/* XXX: hash order check? */
+	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
+	for (i = 0, ent = entries; i < ichdr.count; ent++, i++) {
+		fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr,
+				ent, i, &last_hashval);
+		if (fa)
+			return fa;
+	}
 
 	/*
 	 * Quickly check the freemap information.  Attribute data has to be

[PATCH 2/4] xfs: namecheck attribute names before listing them

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Actually call namecheck on attribute names before we hand them over to
userspace.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.h |    4 ++--
 fs/xfs/xfs_attr_list.c        |   40 ++++++++++++++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 10 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index 7b74e18becff..bb0880057ee3 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -67,8 +67,8 @@ int	xfs_attr3_leaf_add(struct xfs_buf *leaf_buffer,
 				 struct xfs_da_args *args);
 int	xfs_attr3_leaf_remove(struct xfs_buf *leaf_buffer,
 				    struct xfs_da_args *args);
-void	xfs_attr3_leaf_list_int(struct xfs_buf *bp,
-				      struct xfs_attr_list_context *context);
+int	xfs_attr3_leaf_list_int(struct xfs_buf *bp,
+				struct xfs_attr_list_context *context);
 
 /*
  * Routines used for shrinking the Btree.
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 00758fdc2fec..3a1158a1347d 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -84,6 +84,11 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 	    (XFS_ISRESET_CURSOR(cursor) &&
 	     (dp->i_afp->if_bytes + sf->hdr.count * 16) < context->bufsize)) {
 		for (i = 0, sfe = &sf->list[0]; i < sf->hdr.count; i++) {
+			if (!xfs_attr_namecheck(sfe->nameval, sfe->namelen)) {
+				XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
+						 context->dp->i_mount);
+				return -EFSCORRUPTED;
+			}
 			context->put_listent(context,
 					     sfe->flags,
 					     sfe->nameval,
@@ -174,6 +179,11 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 			cursor->hashval = sbp->hash;
 			cursor->offset = 0;
 		}
+		if (!xfs_attr_namecheck(sbp->name, sbp->namelen)) {
+			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
+					 context->dp->i_mount);
+			return -EFSCORRUPTED;
+		}
 		context->put_listent(context,
 				     sbp->flags,
 				     sbp->name,
@@ -284,7 +294,7 @@ xfs_attr_node_list(
 	struct xfs_buf			*bp;
 	struct xfs_inode		*dp = context->dp;
 	struct xfs_mount		*mp = dp->i_mount;
-	int				error;
+	int				error = 0;
 
 	trace_xfs_attr_node_list(context);
 
@@ -358,7 +368,9 @@ xfs_attr_node_list(
 	 */
 	for (;;) {
 		leaf = bp->b_addr;
-		xfs_attr3_leaf_list_int(bp, context);
+		error = xfs_attr3_leaf_list_int(bp, context);
+		if (error)
+			break;
 		xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
 		if (context->seen_enough || leafhdr.forw == 0)
 			break;
@@ -369,13 +381,13 @@ xfs_attr_node_list(
 			return error;
 	}
 	xfs_trans_brelse(context->tp, bp);
-	return 0;
+	return error;
 }
 
 /*
  * Copy out attribute list entries for attr_list(), for leaf attribute lists.
  */
-void
+int
 xfs_attr3_leaf_list_int(
 	struct xfs_buf			*bp,
 	struct xfs_attr_list_context	*context)
@@ -417,7 +429,7 @@ xfs_attr3_leaf_list_int(
 		}
 		if (i == ichdr.count) {
 			trace_xfs_attr_list_notfound(context);
-			return;
+			return 0;
 		}
 	} else {
 		entry = &entries[0];
@@ -457,6 +469,11 @@ xfs_attr3_leaf_list_int(
 			valuelen = be32_to_cpu(name_rmt->valuelen);
 		}
 
+		if (!xfs_attr_namecheck(name, namelen)) {
+			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
+					 context->dp->i_mount);
+			return -EFSCORRUPTED;
+		}
 		context->put_listent(context, entry->flags,
 					      name, namelen, valuelen);
 		if (context->seen_enough)
@@ -464,7 +481,7 @@ xfs_attr3_leaf_list_int(
 		cursor->offset++;
 	}
 	trace_xfs_attr_list_leaf_end(context);
-	return;
+	return 0;
 }
 
 /*
@@ -483,9 +500,9 @@ xfs_attr_leaf_list(xfs_attr_list_context_t *context)
 	if (error)
 		return error;
 
-	xfs_attr3_leaf_list_int(bp, context);
+	error = xfs_attr3_leaf_list_int(bp, context);
 	xfs_trans_brelse(context->tp, bp);
-	return 0;
+	return error;
 }
 
 int
@@ -557,6 +574,13 @@ xfs_attr_put_listent(
 	ASSERT(context->firstu >= sizeof(*alist));
 	ASSERT(context->firstu <= context->bufsize);
 
+	if (!xfs_attr_namecheck(name, namelen)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
+				 context->dp->i_mount);
+		context->seen_enough = -EFSCORRUPTED;
+		return;
+	}
+
 	/*
 	 * Only list entries in the right namespace.
 	 */

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

[Darrick J. Wong]

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

[PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

There are a few places where we return -EIO instead of -EFSCORRUPTED
when we find corrupt metadata.  Fix those places.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c   |    6 +++---
 fs/xfs/xfs_attr_inactive.c |    6 +++---
 fs/xfs/xfs_dquot.c         |    2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 02469d59c787..587889585a23 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1374,7 +1374,7 @@ xfs_bmap_last_before(
 	case XFS_DINODE_FMT_EXTENTS:
 		break;
 	default:
-		return -EIO;
+		return -EFSCORRUPTED;
 	}
 
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
@@ -1475,7 +1475,7 @@ xfs_bmap_last_offset(
 
 	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
 	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS)
-	       return -EIO;
+		return -EFSCORRUPTED;
 
 	error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, &is_empty);
 	if (error || is_empty)
@@ -5864,7 +5864,7 @@ xfs_bmap_insert_extents(
 				del_cursor);
 
 	if (stop_fsb >= got.br_startoff + got.br_blockcount) {
-		error = -EIO;
+		error = -EFSCORRUPTED;
 		goto del_cursor;
 	}
 
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index a640a285cc52..f83f11d929e4 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -209,7 +209,7 @@ xfs_attr3_node_inactive(
 	 */
 	if (level > XFS_DA_NODE_MAXDEPTH) {
 		xfs_trans_brelse(*trans, bp);	/* no locks for later trans */
-		return -EIO;
+		return -EFSCORRUPTED;
 	}
 
 	node = bp->b_addr;
@@ -258,7 +258,7 @@ xfs_attr3_node_inactive(
 			error = xfs_attr3_leaf_inactive(trans, dp, child_bp);
 			break;
 		default:
-			error = -EIO;
+			error = -EFSCORRUPTED;
 			xfs_trans_brelse(*trans, child_bp);
 			break;
 		}
@@ -341,7 +341,7 @@ xfs_attr3_root_inactive(
 		error = xfs_attr3_leaf_inactive(trans, dp, bp);
 		break;
 	default:
-		error = -EIO;
+		error = -EFSCORRUPTED;
 		xfs_trans_brelse(*trans, bp);
 		break;
 	}
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index aeb95e7391c1..2b87c96fb2c0 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1126,7 +1126,7 @@ xfs_qm_dqflush(
 		xfs_buf_relse(bp);
 		xfs_dqfunlock(dqp);
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-		return -EIO;
+		return -EFSCORRUPTED;
 	}
 
 	/* This is the only portion of data that needs to persist */

Re: [PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

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.

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

[Darrick J. Wong]

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

Re: [PATCH 1/4] xfs: check attribute leaf block structure

[Brian Foster]

On Thu, Oct 24, 2019 at 10:14:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add missing structure checks in the attribute leaf verifier.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |   63 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index ec7921e07f69..8dea3a273029 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -232,6 +232,57 @@ xfs_attr3_leaf_hdr_to_disk(
>  	}
>  }
>  
> +static xfs_failaddr_t
> +xfs_attr3_leaf_verify_entry(
> +	struct xfs_mount			*mp,
> +	char					*buf_end,
> +	struct xfs_attr_leafblock		*leaf,
> +	struct xfs_attr3_icleaf_hdr		*leafhdr,
> +	struct xfs_attr_leaf_entry		*ent,
> +	int					idx,
> +	__u32					*last_hashval)
> +{
> +	struct xfs_attr_leaf_name_local		*lentry;
> +	struct xfs_attr_leaf_name_remote	*rentry;
> +	char					*name_end;
> +	unsigned int				nameidx;
> +	unsigned int				namesize;
> +	__u32					hashval;
> +
> +	/* hash order check */
> +	hashval = be32_to_cpu(ent->hashval);
> +	if (hashval < *last_hashval)
> +		return __this_address;
> +	*last_hashval = hashval;
> +
> +	nameidx = be16_to_cpu(ent->nameidx);
> +	if (nameidx < leafhdr->firstused || nameidx >= mp->m_attr_geo->blksize)
> +		return __this_address;
> +
> +	/* Check the name information. */
> +	if (ent->flags & XFS_ATTR_LOCAL) {
> +		lentry = xfs_attr3_leaf_name_local(leaf, idx);
> +		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
> +				be16_to_cpu(lentry->valuelen));
> +		name_end = (char *)lentry + namesize;
> +		if (lentry->namelen == 0)
> +			return __this_address;

I think this reads a little better if we check the lentry value before
we use it (same deal with rentry in the branch below).

Also, why the == 0 checks specifically? Or IOW, might there also be a
sane max value to check some of these fields against?

> +	} else {
> +		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
> +		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
> +		name_end = (char *)rentry + namesize;
> +		if (rentry->namelen == 0)
> +			return __this_address;
> +		if (rentry->valueblk == 0)
> +			return __this_address;

Hmm.. ISTR that it's currently possible to have ->valueblk == 0 on an
incomplete remote attr after a crash. That's not ideal and hopefully
fixed up after the xattr intent stuff lands, but in the meantime I
thought we had code sprinkled around somewhere to fix that up after the
fact. Would this turn that scenario into a metadata I/O error?

Brian

> +	}
> +
> +	if (name_end > buf_end)
> +		return __this_address;
> +
> +	return NULL;
> +}
> +
>  static xfs_failaddr_t
>  xfs_attr3_leaf_verify(
>  	struct xfs_buf			*bp)
> @@ -240,7 +291,10 @@ xfs_attr3_leaf_verify(
>  	struct xfs_mount		*mp = bp->b_mount;
>  	struct xfs_attr_leafblock	*leaf = bp->b_addr;
>  	struct xfs_attr_leaf_entry	*entries;
> +	struct xfs_attr_leaf_entry	*ent;
> +	char				*buf_end;
>  	uint32_t			end;	/* must be 32bit - see below */
> +	__u32				last_hashval = 0;
>  	int				i;
>  	xfs_failaddr_t			fa;
>  
> @@ -273,8 +327,13 @@ xfs_attr3_leaf_verify(
>  	    (char *)bp->b_addr + ichdr.firstused)
>  		return __this_address;
>  
> -	/* XXX: need to range check rest of attr header values */
> -	/* XXX: hash order check? */
> +	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
> +	for (i = 0, ent = entries; i < ichdr.count; ent++, i++) {
> +		fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr,
> +				ent, i, &last_hashval);
> +		if (fa)
> +			return fa;
> +	}
>  
>  	/*
>  	 * Quickly check the freemap information.  Attribute data has to be
> 

Re: [PATCH 2/4] xfs: namecheck attribute names before listing them

[Brian Foster]

On Thu, Oct 24, 2019 at 10:14:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Actually call namecheck on attribute names before we hand them over to
> userspace.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.h |    4 ++--
>  fs/xfs/xfs_attr_list.c        |   40 ++++++++++++++++++++++++++++++++--------
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> 
...
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 00758fdc2fec..3a1158a1347d 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
...
> @@ -174,6 +179,11 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
>  			cursor->hashval = sbp->hash;
>  			cursor->offset = 0;
>  		}
> +		if (!xfs_attr_namecheck(sbp->name, sbp->namelen)) {
> +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> +					 context->dp->i_mount);
> +			return -EFSCORRUPTED;
> +		}

It looks like we still need to handle freeing sbuf in this path.

>  		context->put_listent(context,
>  				     sbp->flags,
>  				     sbp->name,
...
> @@ -557,6 +574,13 @@ xfs_attr_put_listent(
>  	ASSERT(context->firstu >= sizeof(*alist));
>  	ASSERT(context->firstu <= context->bufsize);
>  
> +	if (!xfs_attr_namecheck(name, namelen)) {
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> +				 context->dp->i_mount);
> +		context->seen_enough = -EFSCORRUPTED;
> +		return;
> +	}
> +

Any reason we call this here and the ->put_listent() callers?

Brian

>  	/*
>  	 * Only list entries in the right namespace.
>  	 */
> 

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

[Brian Foster]

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

Re: [PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata

[Brian Foster]

On Thu, Oct 24, 2019 at 10:15:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> There are a few places where we return -EIO instead of -EFSCORRUPTED
> when we find corrupt metadata.  Fix those places.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c   |    6 +++---
>  fs/xfs/xfs_attr_inactive.c |    6 +++---
>  fs/xfs/xfs_dquot.c         |    2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 02469d59c787..587889585a23 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1374,7 +1374,7 @@ xfs_bmap_last_before(
>  	case XFS_DINODE_FMT_EXTENTS:
>  		break;
>  	default:
> -		return -EIO;
> +		return -EFSCORRUPTED;
>  	}
>  
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> @@ -1475,7 +1475,7 @@ xfs_bmap_last_offset(
>  
>  	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
>  	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS)
> -	       return -EIO;
> +		return -EFSCORRUPTED;
>  
>  	error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, &is_empty);
>  	if (error || is_empty)
> @@ -5864,7 +5864,7 @@ xfs_bmap_insert_extents(
>  				del_cursor);
>  
>  	if (stop_fsb >= got.br_startoff + got.br_blockcount) {
> -		error = -EIO;
> +		error = -EFSCORRUPTED;
>  		goto del_cursor;
>  	}
>  
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index a640a285cc52..f83f11d929e4 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -209,7 +209,7 @@ xfs_attr3_node_inactive(
>  	 */
>  	if (level > XFS_DA_NODE_MAXDEPTH) {
>  		xfs_trans_brelse(*trans, bp);	/* no locks for later trans */
> -		return -EIO;
> +		return -EFSCORRUPTED;
>  	}
>  
>  	node = bp->b_addr;
> @@ -258,7 +258,7 @@ xfs_attr3_node_inactive(
>  			error = xfs_attr3_leaf_inactive(trans, dp, child_bp);
>  			break;
>  		default:
> -			error = -EIO;
> +			error = -EFSCORRUPTED;
>  			xfs_trans_brelse(*trans, child_bp);
>  			break;
>  		}
> @@ -341,7 +341,7 @@ xfs_attr3_root_inactive(
>  		error = xfs_attr3_leaf_inactive(trans, dp, bp);
>  		break;
>  	default:
> -		error = -EIO;
> +		error = -EFSCORRUPTED;
>  		xfs_trans_brelse(*trans, bp);
>  		break;
>  	}
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index aeb95e7391c1..2b87c96fb2c0 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -1126,7 +1126,7 @@ xfs_qm_dqflush(
>  		xfs_buf_relse(bp);
>  		xfs_dqfunlock(dqp);
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -		return -EIO;
> +		return -EFSCORRUPTED;
>  	}
>  
>  	/* This is the only portion of data that needs to persist */
> 

Re: [PATCH 2/4] xfs: namecheck attribute names before listing them

[Darrick J. Wong]

On Mon, Oct 28, 2019 at 02:18:57PM -0400, Brian Foster wrote:
> On Thu, Oct 24, 2019 at 10:14:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Actually call namecheck on attribute names before we hand them over to
> > userspace.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr_leaf.h |    4 ++--
> >  fs/xfs/xfs_attr_list.c        |   40 ++++++++++++++++++++++++++++++++--------
> >  2 files changed, 34 insertions(+), 10 deletions(-)
> > 
> > 
> ...
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 00758fdc2fec..3a1158a1347d 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> ...
> > @@ -174,6 +179,11 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
> >  			cursor->hashval = sbp->hash;
> >  			cursor->offset = 0;
> >  		}
> > +		if (!xfs_attr_namecheck(sbp->name, sbp->namelen)) {
> > +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > +					 context->dp->i_mount);
> > +			return -EFSCORRUPTED;
> > +		}
> 
> It looks like we still need to handle freeing sbuf in this path.

Fixed.  Good catch! :)

> >  		context->put_listent(context,
> >  				     sbp->flags,
> >  				     sbp->name,
> ...
> > @@ -557,6 +574,13 @@ xfs_attr_put_listent(
> >  	ASSERT(context->firstu >= sizeof(*alist));
> >  	ASSERT(context->firstu <= context->bufsize);
> >  
> > +	if (!xfs_attr_namecheck(name, namelen)) {
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > +				 context->dp->i_mount);
> > +		context->seen_enough = -EFSCORRUPTED;
> > +		return;
> > +	}
> > +
> 
> Any reason we call this here and the ->put_listent() callers?

Oops.  I guess I got overzealous.

--D

> Brian
> 
> >  	/*
> >  	 * Only list entries in the right namespace.
> >  	 */
> > 
> 

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

[Darrick J. Wong]

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

Re: [PATCH 1/4] xfs: check attribute leaf block structure

[Darrick J. Wong]

On Mon, Oct 28, 2019 at 02:18:13PM -0400, Brian Foster wrote:
> On Thu, Oct 24, 2019 at 10:14:53PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add missing structure checks in the attribute leaf verifier.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr_leaf.c |   63 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 61 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index ec7921e07f69..8dea3a273029 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -232,6 +232,57 @@ xfs_attr3_leaf_hdr_to_disk(
> >  	}
> >  }
> >  
> > +static xfs_failaddr_t
> > +xfs_attr3_leaf_verify_entry(
> > +	struct xfs_mount			*mp,
> > +	char					*buf_end,
> > +	struct xfs_attr_leafblock		*leaf,
> > +	struct xfs_attr3_icleaf_hdr		*leafhdr,
> > +	struct xfs_attr_leaf_entry		*ent,
> > +	int					idx,
> > +	__u32					*last_hashval)
> > +{
> > +	struct xfs_attr_leaf_name_local		*lentry;
> > +	struct xfs_attr_leaf_name_remote	*rentry;
> > +	char					*name_end;
> > +	unsigned int				nameidx;
> > +	unsigned int				namesize;
> > +	__u32					hashval;
> > +
> > +	/* hash order check */
> > +	hashval = be32_to_cpu(ent->hashval);
> > +	if (hashval < *last_hashval)
> > +		return __this_address;
> > +	*last_hashval = hashval;
> > +
> > +	nameidx = be16_to_cpu(ent->nameidx);
> > +	if (nameidx < leafhdr->firstused || nameidx >= mp->m_attr_geo->blksize)
> > +		return __this_address;
> > +
> > +	/* Check the name information. */
> > +	if (ent->flags & XFS_ATTR_LOCAL) {
> > +		lentry = xfs_attr3_leaf_name_local(leaf, idx);
> > +		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
> > +				be16_to_cpu(lentry->valuelen));
> > +		name_end = (char *)lentry + namesize;
> > +		if (lentry->namelen == 0)
> > +			return __this_address;
> 
> I think this reads a little better if we check the lentry value before
> we use it (same deal with rentry in the branch below).
> 
> Also, why the == 0 checks specifically? Or IOW, might there also be a
> sane max value to check some of these fields against?

Attributes have a maximum name length of 255 characters, and the ondisk
namelen field is u8, so it's never possible to exceed the maximum.

> > +	} else {
> > +		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
> > +		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
> > +		name_end = (char *)rentry + namesize;
> > +		if (rentry->namelen == 0)
> > +			return __this_address;
> > +		if (rentry->valueblk == 0)
> > +			return __this_address;
> 
> Hmm.. ISTR that it's currently possible to have ->valueblk == 0 on an
> incomplete remote attr after a crash. That's not ideal and hopefully
> fixed up after the xattr intent stuff lands, but in the meantime I
> thought we had code sprinkled around somewhere to fix that up after the
> fact. Would this turn that scenario into a metadata I/O error?

<urk> Yes, it would.  I'll fix that.

--D

> 
> Brian
> 
> > +	}
> > +
> > +	if (name_end > buf_end)
> > +		return __this_address;
> > +
> > +	return NULL;
> > +}
> > +
> >  static xfs_failaddr_t
> >  xfs_attr3_leaf_verify(
> >  	struct xfs_buf			*bp)
> > @@ -240,7 +291,10 @@ xfs_attr3_leaf_verify(
> >  	struct xfs_mount		*mp = bp->b_mount;
> >  	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> >  	struct xfs_attr_leaf_entry	*entries;
> > +	struct xfs_attr_leaf_entry	*ent;
> > +	char				*buf_end;
> >  	uint32_t			end;	/* must be 32bit - see below */
> > +	__u32				last_hashval = 0;
> >  	int				i;
> >  	xfs_failaddr_t			fa;
> >  
> > @@ -273,8 +327,13 @@ xfs_attr3_leaf_verify(
> >  	    (char *)bp->b_addr + ichdr.firstused)
> >  		return __this_address;
> >  
> > -	/* XXX: need to range check rest of attr header values */
> > -	/* XXX: hash order check? */
> > +	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
> > +	for (i = 0, ent = entries; i < ichdr.count; ent++, i++) {
> > +		fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr,
> > +				ent, i, &last_hashval);
> > +		if (fa)
> > +			return fa;
> > +	}
> >  
> >  	/*
> >  	 * Quickly check the freemap information.  Attribute data has to be
> > 
> 

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

[Christoph Hellwig]

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.

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

[Darrick J. Wong]

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

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

[Christoph Hellwig]

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.

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

[Darrick J. Wong]

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