| Message ID | 20180115200527.GE5602@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Jan 15, 2018 at 12:05:27PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > In the attribute leaf verifier, we can check for obviously bad values of > firstused and count so that later attempts at lasthash don't run off the > end of the memory buffer. Found by ones fuzzing hdr.count in xfs/400 with > KASAN. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > v2: strengthen checking, clarify what we're testing via comments > --- Ok, digging more into the code I see that firstused refers to the lowest ->nameidx of the xfs_attr_leaf_entry's in the block. On insert, ->nameidx is constructed from freemap.base, which starts at just beyond the block header. freemap.base+freemap.size essentially refers to a raw block offset where the name/value info is placed, so I think I just misread that code the first time around. This makes sense and the cleanups look good. Thanks for the clarification. One more small question.. > fs/xfs/libxfs/xfs_attr_leaf.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 6fddce7..f1a1c60 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk( > > static xfs_failaddr_t > xfs_attr3_leaf_verify( > - struct xfs_buf *bp) > + struct xfs_buf *bp) > { > - struct xfs_mount *mp = bp->b_target->bt_mount; > - struct xfs_attr_leafblock *leaf = bp->b_addr; > - struct xfs_perag *pag = bp->b_pag; > - struct xfs_attr3_icleaf_hdr ichdr; > + struct xfs_attr3_icleaf_hdr ichdr; > + struct xfs_mount *mp = bp->b_target->bt_mount; > + struct xfs_attr_leafblock *leaf = bp->b_addr; > + struct xfs_perag *pag = bp->b_pag; > + struct xfs_attr_leaf_entry *entries; > > xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf); > > @@ -282,6 +283,21 @@ xfs_attr3_leaf_verify( > if (pag && pag->pagf_init && ichdr.count == 0) > return __this_address; > > + /* > + * firstused is the block offset of the first name info structure. > + * Make sure it doesn't go off the block or crash into the header. > + */ > + if (ichdr.firstused > mp->m_attr_geo->blksize) > + return __this_address; > + if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf)) > + return __this_address; > + > + /* Make sure the entries array doesn't crash into the name info. */ > + entries = xfs_attr3_leaf_entryp(bp->b_addr); > + if ((char *)&entries[ichdr.count] >= > + (char *)bp->b_addr + ichdr.firstused) > + return __this_address; > + Can the end of the entries list align with the first namelist object if the block is full, for example? E.g., is '&entries[ichdr.count] == bp->b_addr + ichdr.firstused' a sane possibility? That aside, this looks good to me: Reviewed-by: Brian Foster <bfoster@redhat.com> > /* XXX: need to range check rest of attr header values */ > /* XXX: hash order check? */ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 16, 2018 at 07:50:45AM -0500, Brian Foster wrote: > On Mon, Jan 15, 2018 at 12:05:27PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > In the attribute leaf verifier, we can check for obviously bad values of > > firstused and count so that later attempts at lasthash don't run off the > > end of the memory buffer. Found by ones fuzzing hdr.count in xfs/400 with > > KASAN. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > v2: strengthen checking, clarify what we're testing via comments > > --- > > Ok, digging more into the code I see that firstused refers to the lowest > ->nameidx of the xfs_attr_leaf_entry's in the block. On insert, > ->nameidx is constructed from freemap.base, which starts at just beyond > the block header. freemap.base+freemap.size essentially refers to a raw > block offset where the name/value info is placed, so I think I just > misread that code the first time around. This makes sense and the > cleanups look good. Thanks for the clarification. > > One more small question.. > > > fs/xfs/libxfs/xfs_attr_leaf.c | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > > index 6fddce7..f1a1c60 100644 > > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > > @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk( > > > > static xfs_failaddr_t > > xfs_attr3_leaf_verify( > > - struct xfs_buf *bp) > > + struct xfs_buf *bp) > > { > > - struct xfs_mount *mp = bp->b_target->bt_mount; > > - struct xfs_attr_leafblock *leaf = bp->b_addr; > > - struct xfs_perag *pag = bp->b_pag; > > - struct xfs_attr3_icleaf_hdr ichdr; > > + struct xfs_attr3_icleaf_hdr ichdr; > > + struct xfs_mount *mp = bp->b_target->bt_mount; > > + struct xfs_attr_leafblock *leaf = bp->b_addr; > > + struct xfs_perag *pag = bp->b_pag; > > + struct xfs_attr_leaf_entry *entries; > > > > xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf); > > > > @@ -282,6 +283,21 @@ xfs_attr3_leaf_verify( > > if (pag && pag->pagf_init && ichdr.count == 0) > > return __this_address; > > > > + /* > > + * firstused is the block offset of the first name info structure. > > + * Make sure it doesn't go off the block or crash into the header. > > + */ > > + if (ichdr.firstused > mp->m_attr_geo->blksize) > > + return __this_address; > > + if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf)) > > + return __this_address; > > + > > + /* Make sure the entries array doesn't crash into the name info. */ > > + entries = xfs_attr3_leaf_entryp(bp->b_addr); > > + if ((char *)&entries[ichdr.count] >= > > + (char *)bp->b_addr + ichdr.firstused) > > + return __this_address; > > + > > Can the end of the entries list align with the first namelist object if > the block is full, for example? E.g., is '&entries[ichdr.count] == > bp->b_addr + ichdr.firstused' a sane possibility? Yes. Good catch! I'll change the '>=' to '>'. --D > That aside, this looks good to me: > > Reviewed-by: Brian Foster <bfoster@redhat.com> > > > /* XXX: need to range check rest of attr header values */ > > /* XXX: hash order check? */ > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 6fddce7..f1a1c60 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -249,12 +249,13 @@ xfs_attr3_leaf_hdr_to_disk( static xfs_failaddr_t xfs_attr3_leaf_verify( - struct xfs_buf *bp) + struct xfs_buf *bp) { - struct xfs_mount *mp = bp->b_target->bt_mount; - struct xfs_attr_leafblock *leaf = bp->b_addr; - struct xfs_perag *pag = bp->b_pag; - struct xfs_attr3_icleaf_hdr ichdr; + struct xfs_attr3_icleaf_hdr ichdr; + struct xfs_mount *mp = bp->b_target->bt_mount; + struct xfs_attr_leafblock *leaf = bp->b_addr; + struct xfs_perag *pag = bp->b_pag; + struct xfs_attr_leaf_entry *entries; xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf); @@ -282,6 +283,21 @@ xfs_attr3_leaf_verify( if (pag && pag->pagf_init && ichdr.count == 0) return __this_address; + /* + * firstused is the block offset of the first name info structure. + * Make sure it doesn't go off the block or crash into the header. + */ + if (ichdr.firstused > mp->m_attr_geo->blksize) + return __this_address; + if (ichdr.firstused < xfs_attr3_leaf_hdr_size(leaf)) + return __this_address; + + /* Make sure the entries array doesn't crash into the name info. */ + entries = xfs_attr3_leaf_entryp(bp->b_addr); + if ((char *)&entries[ichdr.count] >= + (char *)bp->b_addr + ichdr.firstused) + return __this_address; + /* XXX: need to range check rest of attr header values */ /* XXX: hash order check? */