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