* [PATCH v2 0/4] xfs: more metadata verifier tightening
@ 2019-10-29 4:03 Darrick J. Wong
2019-10-29 4:03 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-29 4:03 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] xfs: check attribute leaf block structure
2019-10-29 4:03 [PATCH v2 0/4] xfs: more metadata verifier tightening Darrick J. Wong
@ 2019-10-29 4:03 ` Darrick J. Wong
2019-10-29 10:03 ` Brian Foster
2019-10-29 4:03 ` [PATCH 2/4] xfs: namecheck attribute names before listing them Darrick J. Wong
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-29 4:03 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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 | 67 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index f0089e862216..bb80a01bcca9 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -232,6 +232,61 @@ 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. The namelen fields are u8 so we can't
+ * possibly exceed the maximum name length of 255 bytes.
+ */
+ 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 (!(ent->flags & XFS_ATTR_INCOMPLETE) &&
+ 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 +295,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 +331,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] xfs: namecheck attribute names before listing them
2019-10-29 4:03 [PATCH v2 0/4] xfs: more metadata verifier tightening Darrick J. Wong
2019-10-29 4:03 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
@ 2019-10-29 4:03 ` Darrick J. Wong
2019-10-29 10:03 ` Brian Foster
2019-10-29 4:04 ` [PATCH 3/4] xfs: namecheck directory entry " Darrick J. Wong
2019-10-29 4:04 ` [PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata Darrick J. Wong
3 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-29 4:03 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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 | 60 +++++++++++++++++++++++++++--------------
2 files changed, 41 insertions(+), 23 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..c02f22d50e45 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -49,14 +49,16 @@ xfs_attr_shortform_compare(const void *a, const void *b)
* we can begin returning them to the user.
*/
static int
-xfs_attr_shortform_list(xfs_attr_list_context_t *context)
+xfs_attr_shortform_list(
+ struct xfs_attr_list_context *context)
{
- attrlist_cursor_kern_t *cursor;
- xfs_attr_sf_sort_t *sbuf, *sbp;
- xfs_attr_shortform_t *sf;
- xfs_attr_sf_entry_t *sfe;
- xfs_inode_t *dp;
- int sbsize, nsbuf, count, i;
+ struct attrlist_cursor_kern *cursor;
+ struct xfs_attr_sf_sort *sbuf, *sbp;
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ struct xfs_inode *dp;
+ int sbsize, nsbuf, count, i;
+ int error = 0;
ASSERT(context != NULL);
dp = context->dp;
@@ -84,6 +86,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,
@@ -161,10 +168,8 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
break;
}
}
- if (i == nsbuf) {
- kmem_free(sbuf);
- return 0;
- }
+ if (i == nsbuf)
+ goto out;
/*
* Loop putting entries into the user buffer.
@@ -174,6 +179,12 @@ 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);
+ error = -EFSCORRUPTED;
+ goto out;
+ }
context->put_listent(context,
sbp->flags,
sbp->name,
@@ -183,9 +194,9 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
break;
cursor->offset++;
}
-
+out:
kmem_free(sbuf);
- return 0;
+ return error;
}
/*
@@ -284,7 +295,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 +369,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 +382,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 +430,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 +470,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 +482,7 @@ xfs_attr3_leaf_list_int(
cursor->offset++;
}
trace_xfs_attr_list_leaf_end(context);
- return;
+ return 0;
}
/*
@@ -483,9 +501,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] xfs: namecheck directory entry names before listing them
2019-10-29 4:03 [PATCH v2 0/4] xfs: more metadata verifier tightening Darrick J. Wong
2019-10-29 4:03 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
2019-10-29 4:03 ` [PATCH 2/4] xfs: namecheck attribute names before listing them Darrick J. Wong
@ 2019-10-29 4:04 ` Darrick J. Wong
2019-10-29 10:04 ` Brian Foster
2019-10-29 4:04 ` [PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata Darrick J. Wong
3 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-29 4:04 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dir2_readdir.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 283df898dd9f..a0bec0931f3b 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,12 +214,16 @@ 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);
+ error = -EFSCORRUPTED;
+ goto out_rele;
+ }
if (!dir_emit(ctx, (char *)dep->name, dep->namelen,
be64_to_cpu(dep->inumber),
- xfs_dir3_get_dtype(dp->i_mount, filetype))) {
- xfs_trans_brelse(args->trans, bp);
- return 0;
- }
+ xfs_dir3_get_dtype(dp->i_mount, filetype)))
+ goto out_rele;
}
/*
@@ -222,8 +232,9 @@ xfs_dir2_block_getdents(
*/
ctx->pos = xfs_dir2_db_off_to_dataptr(geo, geo->datablk + 1, 0) &
0x7fffffff;
+out_rele:
xfs_trans_brelse(args->trans, bp);
- return 0;
+ return error;
}
/*
@@ -456,6 +467,12 @@ 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);
+ error = -EFSCORRUPTED;
+ break;
+ }
if (!dir_emit(ctx, (char *)dep->name, dep->namelen,
be64_to_cpu(dep->inumber),
xfs_dir3_get_dtype(dp->i_mount, filetype)))
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata
2019-10-29 4:03 [PATCH v2 0/4] xfs: more metadata verifier tightening Darrick J. Wong
` (2 preceding siblings ...)
2019-10-29 4:04 ` [PATCH 3/4] xfs: namecheck directory entry " Darrick J. Wong
@ 2019-10-29 4:04 ` Darrick J. Wong
3 siblings, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-29 4:04 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster, hch
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: Christoph Hellwig <hch@lst.de>
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 */
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] xfs: check attribute leaf block structure
2019-10-29 4:03 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
@ 2019-10-29 10:03 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2019-10-29 10:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, Oct 28, 2019 at 09:03:48PM -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>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_attr_leaf.c | 67 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 65 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index f0089e862216..bb80a01bcca9 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -232,6 +232,61 @@ 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. The namelen fields are u8 so we can't
> + * possibly exceed the maximum name length of 255 bytes.
> + */
> + 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 (!(ent->flags & XFS_ATTR_INCOMPLETE) &&
> + 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 +295,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 +331,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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] xfs: namecheck attribute names before listing them
2019-10-29 4:03 ` [PATCH 2/4] xfs: namecheck attribute names before listing them Darrick J. Wong
@ 2019-10-29 10:03 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2019-10-29 10:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, Oct 28, 2019 at 09:03:58PM -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>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_attr_leaf.h | 4 +--
> fs/xfs/xfs_attr_list.c | 60 +++++++++++++++++++++++++++--------------
> 2 files changed, 41 insertions(+), 23 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..c02f22d50e45 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -49,14 +49,16 @@ xfs_attr_shortform_compare(const void *a, const void *b)
> * we can begin returning them to the user.
> */
> static int
> -xfs_attr_shortform_list(xfs_attr_list_context_t *context)
> +xfs_attr_shortform_list(
> + struct xfs_attr_list_context *context)
> {
> - attrlist_cursor_kern_t *cursor;
> - xfs_attr_sf_sort_t *sbuf, *sbp;
> - xfs_attr_shortform_t *sf;
> - xfs_attr_sf_entry_t *sfe;
> - xfs_inode_t *dp;
> - int sbsize, nsbuf, count, i;
> + struct attrlist_cursor_kern *cursor;
> + struct xfs_attr_sf_sort *sbuf, *sbp;
> + struct xfs_attr_shortform *sf;
> + struct xfs_attr_sf_entry *sfe;
> + struct xfs_inode *dp;
> + int sbsize, nsbuf, count, i;
> + int error = 0;
>
> ASSERT(context != NULL);
> dp = context->dp;
> @@ -84,6 +86,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,
> @@ -161,10 +168,8 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
> break;
> }
> }
> - if (i == nsbuf) {
> - kmem_free(sbuf);
> - return 0;
> - }
> + if (i == nsbuf)
> + goto out;
>
> /*
> * Loop putting entries into the user buffer.
> @@ -174,6 +179,12 @@ 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);
> + error = -EFSCORRUPTED;
> + goto out;
> + }
> context->put_listent(context,
> sbp->flags,
> sbp->name,
> @@ -183,9 +194,9 @@ xfs_attr_shortform_list(xfs_attr_list_context_t *context)
> break;
> cursor->offset++;
> }
> -
> +out:
> kmem_free(sbuf);
> - return 0;
> + return error;
> }
>
> /*
> @@ -284,7 +295,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 +369,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 +382,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 +430,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 +470,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 +482,7 @@ xfs_attr3_leaf_list_int(
> cursor->offset++;
> }
> trace_xfs_attr_list_leaf_end(context);
> - return;
> + return 0;
> }
>
> /*
> @@ -483,9 +501,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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] xfs: namecheck directory entry names before listing them
2019-10-29 4:04 ` [PATCH 3/4] xfs: namecheck directory entry " Darrick J. Wong
@ 2019-10-29 10:04 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2019-10-29 10:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, Oct 28, 2019 at 09:04: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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_dir2_readdir.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index 283df898dd9f..a0bec0931f3b 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,12 +214,16 @@ 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);
> + error = -EFSCORRUPTED;
> + goto out_rele;
> + }
> if (!dir_emit(ctx, (char *)dep->name, dep->namelen,
> be64_to_cpu(dep->inumber),
> - xfs_dir3_get_dtype(dp->i_mount, filetype))) {
> - xfs_trans_brelse(args->trans, bp);
> - return 0;
> - }
> + xfs_dir3_get_dtype(dp->i_mount, filetype)))
> + goto out_rele;
> }
>
> /*
> @@ -222,8 +232,9 @@ xfs_dir2_block_getdents(
> */
> ctx->pos = xfs_dir2_db_off_to_dataptr(geo, geo->datablk + 1, 0) &
> 0x7fffffff;
> +out_rele:
> xfs_trans_brelse(args->trans, bp);
> - return 0;
> + return error;
> }
>
> /*
> @@ -456,6 +467,12 @@ 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);
> + error = -EFSCORRUPTED;
> + break;
> + }
> if (!dir_emit(ctx, (char *)dep->name, dep->namelen,
> be64_to_cpu(dep->inumber),
> xfs_dir3_get_dtype(dp->i_mount, filetype)))
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] xfs: check attribute leaf block structure
2019-10-28 18:18 ` Brian Foster
@ 2019-10-28 18:27 ` Darrick J. Wong
0 siblings, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-28 18:27 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
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
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] xfs: check attribute leaf block structure
2019-10-25 5:14 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
@ 2019-10-28 18:18 ` Brian Foster
2019-10-28 18:27 ` Darrick J. Wong
0 siblings, 1 reply; 11+ messages in thread
From: Brian Foster @ 2019-10-28 18:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] xfs: check attribute leaf block structure
2019-10-25 5:14 [PATCH 0/4] xfs: more metadata verifier tightening Darrick J. Wong
@ 2019-10-25 5:14 ` Darrick J. Wong
2019-10-28 18:18 ` Brian Foster
0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2019-10-25 5:14 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-29 10:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-29 4:03 [PATCH v2 0/4] xfs: more metadata verifier tightening Darrick J. Wong
2019-10-29 4:03 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
2019-10-29 10:03 ` Brian Foster
2019-10-29 4:03 ` [PATCH 2/4] xfs: namecheck attribute names before listing them Darrick J. Wong
2019-10-29 10:03 ` Brian Foster
2019-10-29 4:04 ` [PATCH 3/4] xfs: namecheck directory entry " Darrick J. Wong
2019-10-29 10:04 ` Brian Foster
2019-10-29 4:04 ` [PATCH 4/4] xfs: replace -EIO with -EFSCORRUPTED for corrupt metadata Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2019-10-25 5:14 [PATCH 0/4] xfs: more metadata verifier tightening Darrick J. Wong
2019-10-25 5:14 ` [PATCH 1/4] xfs: check attribute leaf block structure Darrick J. Wong
2019-10-28 18:18 ` Brian Foster
2019-10-28 18:27 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).