On Thu, Oct 24, 2019 at 10:14:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Add missing structure checks in the attribute leaf verifier.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> 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
>
