Message ID | 157198049955.2873445.974102983711142585.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: more metadata verifier tightening | expand |
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. > */ >
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 --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. */