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