diff mbox series

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

Message ID 157198049955.2873445.974102983711142585.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:14 a.m. UTC 
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(-)

Comments

Brian Foster Oct. 28, 2019, 6:18 p.m. UTC | #1 
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.
>  	 */
>
Darrick J. Wong Oct. 28, 2019, 6:22 p.m. UTC | #2 
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.
> >  	 */
> > 
>
diff mbox series

Patch

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.
 	 */