| Message ID | 20160819203022.GD8268@birch.djwong.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Aug 19, 2016 at 01:30:22PM -0700, Darrick J. Wong wrote: > If the caller passes in a cursor to a zero-height btree (which is > impossible), we never set block to anything but NULL, which causes the > later dereference of it to crash. Instead, just return -EFSCORRUPTED. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Did something actually cause this to happen? Brian > fs/xfs/libxfs/xfs_btree.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c > index 64fd847..4bffea4 100644 > --- a/fs/xfs/libxfs/xfs_btree.c > +++ b/fs/xfs/libxfs/xfs_btree.c > @@ -1814,6 +1814,10 @@ xfs_btree_lookup( > > XFS_BTREE_STATS_INC(cur, lookup); > > + /* No such thing as a zero-level tree. */ > + if (cur->bc_nlevels == 0) > + return -EFSCORRUPTED; > + > block = NULL; > keyno = 0; > > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs
On Wed, Aug 24, 2016 at 02:01:55PM -0400, Brian Foster wrote: > On Fri, Aug 19, 2016 at 01:30:22PM -0700, Darrick J. Wong wrote: > > If the caller passes in a cursor to a zero-height btree (which is > > impossible), we never set block to anything but NULL, which causes the > > later dereference of it to crash. Instead, just return -EFSCORRUPTED. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > Did something actually cause this to happen? Well, if you took a reflink xfs that was formatted before we added the rmap/refcount btree block counters to the AGF and then tried to mount it after will blow up like this, because the refcount_level field got moved to somewhere that's zero in the old image. Anyone being malicious with xfs_db can also do this to any other _level field. (Yay automounting!) --D > > Brian > > > fs/xfs/libxfs/xfs_btree.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c > > index 64fd847..4bffea4 100644 > > --- a/fs/xfs/libxfs/xfs_btree.c > > +++ b/fs/xfs/libxfs/xfs_btree.c > > @@ -1814,6 +1814,10 @@ xfs_btree_lookup( > > > > XFS_BTREE_STATS_INC(cur, lookup); > > > > + /* No such thing as a zero-level tree. */ > > + if (cur->bc_nlevels == 0) > > + return -EFSCORRUPTED; > > + > > block = NULL; > > keyno = 0; > > > > > > _______________________________________________ > > xfs mailing list > > xfs@oss.sgi.com > > http://oss.sgi.com/mailman/listinfo/xfs
On Fri, Aug 19, 2016 at 01:30:22PM -0700, Darrick J. Wong wrote: > If the caller passes in a cursor to a zero-height btree (which is > impossible), we never set block to anything but NULL, which causes the > later dereference of it to crash. Instead, just return -EFSCORRUPTED. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> maybe also throw in an unlikely notation, though..
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c index 64fd847..4bffea4 100644 --- a/fs/xfs/libxfs/xfs_btree.c +++ b/fs/xfs/libxfs/xfs_btree.c @@ -1814,6 +1814,10 @@ xfs_btree_lookup( XFS_BTREE_STATS_INC(cur, lookup); + /* No such thing as a zero-level tree. */ + if (cur->bc_nlevels == 0) + return -EFSCORRUPTED; + block = NULL; keyno = 0;
If the caller passes in a cursor to a zero-height btree (which is impossible), we never set block to anything but NULL, which causes the later dereference of it to crash. Instead, just return -EFSCORRUPTED. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/libxfs/xfs_btree.c | 4 ++++ 1 file changed, 4 insertions(+)