| Message ID | 148419041963.32674.9938209133184119040.stgit@birch.djwong.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 1/11/17 9:06 PM, Darrick J. Wong wrote: > In xfs_repair, the inodelog, sectlog, and dirblklog values are read > directly into the xfs_mount structure without any sanity checking by the > verifier. This results in xfs_repair segfaulting when those fields have > ridiculously high values because the pointer arithmetic runs us off the > end of the metadata buffers. Therefore, reject the superblock if these > values are garbage and try to find one of the other ones. > > Also clean up the dblocks checking to use the relevant macros and remove > a bogus ASSERT that crashes repair when sunit is set but swidth isn't. > > The superblock field fuzzer (xfs/1301) triggers all these segfaults. ok, thinking out loud below. Mostly fine but question at the end. > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > repair/sb.c | 19 ++++++++++++++----- > repair/xfs_repair.c | 5 ++++- > 2 files changed, 18 insertions(+), 6 deletions(-) > > > diff --git a/repair/sb.c b/repair/sb.c > index 004702c..d784420 100644 > --- a/repair/sb.c > +++ b/repair/sb.c > @@ -395,21 +395,26 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) > /* sanity check ag count, size fields against data size field */ > > if (sb->sb_dblocks == 0 || > - sb->sb_dblocks > > - ((__uint64_t)sb->sb_agcount * sb->sb_agblocks) || > - sb->sb_dblocks < > - ((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks > - + XFS_MIN_AG_BLOCKS)) > + sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) || > + sb->sb_dblocks < XFS_MIN_DBLOCKS(sb)) Ok so this is just proper macro replacement, all good. > return(XR_BAD_FS_SIZE_DATA); > > if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks)) > return(XR_BAD_FS_SIZE_DATA); > > + if (sb->sb_inodelog < XFS_DINODE_MIN_LOG || > + sb->sb_inodelog > XFS_DINODE_MAX_LOG || > + sb->sb_inodelog != libxfs_log2_roundup(sb->sb_inodesize)) > + return XR_BAD_INO_SIZE_DATA; > + missing parentheses on the return. (I kid!) Ok, we haven't checked out inodesize yet, so if it's wrong, the above might return XR_BAD_INO_SIZE_DATA even if sb_inodelog is ok. I suppose that's fine. hm at some point I wonder if this should more closely match xfs_mount_validate_sb (or vice versa) That function does: sbp->sb_inodesize != (1 << sbp->sb_inodelog) || which is the reverse I guess. > if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE || > sb->sb_inodesize > XFS_DINODE_MAX_SIZE || > sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize)) > return(XR_BAD_INO_SIZE_DATA); Ok, this doesn't cross-check inodesize vs. inodelog, but I guess that's already done above. > + if (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) > + return XR_BAD_INO_SIZE_DATA; > + oh yeah that too. I do kind of wonder if we shouldn't just match that mount code more closely, and just do if (sbp->sb_inodesize < XFS_DINODE_MIN_SIZE || sbp->sb_inodesize > XFS_DINODE_MAX_SIZE || sbp->sb_inodelog < XFS_DINODE_MIN_LOG || sbp->sb_inodelog > XFS_DINODE_MAX_LOG || sbp->sb_inodesize != (1 << sbp->sb_inodelog) || sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE || sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) || (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)) return XR_BAD_INO_SIZE_DATA; > if (xfs_sb_version_hassector(sb)) { > > /* check to make sure log sector is legal 2^N, 9 <= N <= 15 */ > @@ -494,6 +499,10 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) > return(XR_BAD_SB_WIDTH); > } > > + /* Directory block log */ > + if (sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG) > + return XR_BAD_FS_SIZE_DATA; > + OK anyway, if any of this fails we go and take a vote from secondaries, should be fine. > return(XR_OK); > } > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > index 5c79fd9..8d4be83 100644 > --- a/repair/xfs_repair.c > +++ b/repair/xfs_repair.c > @@ -621,7 +621,10 @@ is_multidisk_filesystem( > if (!sbp->sb_unit) > return false; > > - ASSERT(sbp->sb_width); > + /* Stripe unit but no stripe width? Something's funny here... */ > + if (!sbp->sb_width) > + return false; > + verify_sb did already sanitize this... /* * verify stripe alignment fields if present */ if (xfs_sb_version_hasdalign(sb)) { if ((!sb->sb_unit && sb->sb_width) || (sb->sb_unit && sb->sb_agblocks % sb->sb_unit)) return(XR_BAD_SB_UNIT); if ((sb->sb_unit && !sb->sb_width) || (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit)) return(XR_BAD_SB_WIDTH); } so we do this when we start up: main get_sb verify_sb ... is_multidisk_filesystem by then it should have been ok, what path did you hit this on? Seems like we shouldn't be making decisions about "is multidisk" here, we should be saying "bad superblock" somewhere earlier, no? I mean, bailing with false is fine I /guess/ but I think the ASSERT implies that we should have already verified that both or none are set... -Eric > return true; > } > > > -- > 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 Fri, Jan 13, 2017 at 08:13:17PM -0600, Eric Sandeen wrote: > On 1/11/17 9:06 PM, Darrick J. Wong wrote: > > In xfs_repair, the inodelog, sectlog, and dirblklog values are read > > directly into the xfs_mount structure without any sanity checking by the > > verifier. This results in xfs_repair segfaulting when those fields have > > ridiculously high values because the pointer arithmetic runs us off the > > end of the metadata buffers. Therefore, reject the superblock if these > > values are garbage and try to find one of the other ones. > > > > Also clean up the dblocks checking to use the relevant macros and remove > > a bogus ASSERT that crashes repair when sunit is set but swidth isn't. > > > > The superblock field fuzzer (xfs/1301) triggers all these segfaults. > > ok, thinking out loud below. Mostly fine but question at the end. > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > repair/sb.c | 19 ++++++++++++++----- > > repair/xfs_repair.c | 5 ++++- > > 2 files changed, 18 insertions(+), 6 deletions(-) > > > > > > diff --git a/repair/sb.c b/repair/sb.c > > index 004702c..d784420 100644 > > --- a/repair/sb.c > > +++ b/repair/sb.c > > @@ -395,21 +395,26 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) > > /* sanity check ag count, size fields against data size field */ > > > > if (sb->sb_dblocks == 0 || > > - sb->sb_dblocks > > > - ((__uint64_t)sb->sb_agcount * sb->sb_agblocks) || > > - sb->sb_dblocks < > > - ((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks > > - + XFS_MIN_AG_BLOCKS)) > > + sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) || > > + sb->sb_dblocks < XFS_MIN_DBLOCKS(sb)) > > Ok so this is just proper macro replacement, all good. > > > return(XR_BAD_FS_SIZE_DATA); > > > > if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks)) > > return(XR_BAD_FS_SIZE_DATA); > > > > + if (sb->sb_inodelog < XFS_DINODE_MIN_LOG || > > + sb->sb_inodelog > XFS_DINODE_MAX_LOG || > > + sb->sb_inodelog != libxfs_log2_roundup(sb->sb_inodesize)) > > + return XR_BAD_INO_SIZE_DATA; > > + > > missing parentheses on the return. (I kid!) :P > Ok, we haven't checked out inodesize yet, so if it's wrong, the above > might return XR_BAD_INO_SIZE_DATA even if sb_inodelog is ok. > I suppose that's fine. > > hm at some point I wonder if this should more closely match > xfs_mount_validate_sb (or vice versa) > > That function does: > > sbp->sb_inodesize != (1 << sbp->sb_inodelog) || > > which is the reverse I guess. > > > if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE || > > sb->sb_inodesize > XFS_DINODE_MAX_SIZE || > > sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize)) > > return(XR_BAD_INO_SIZE_DATA); > > Ok, this doesn't cross-check inodesize vs. inodelog, but I guess that's > already done above. > > > + if (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) > > + return XR_BAD_INO_SIZE_DATA; > > + > > oh yeah that too. > > I do kind of wonder if we shouldn't just match that mount code more > closely, and just do > > if (sbp->sb_inodesize < XFS_DINODE_MIN_SIZE || > sbp->sb_inodesize > XFS_DINODE_MAX_SIZE || > sbp->sb_inodelog < XFS_DINODE_MIN_LOG || > sbp->sb_inodelog > XFS_DINODE_MAX_LOG || > sbp->sb_inodesize != (1 << sbp->sb_inodelog) || > sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE || > sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) || > (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)) > return XR_BAD_INO_SIZE_DATA; Ick, yes, I'll just replace all that with this. > > if (xfs_sb_version_hassector(sb)) { > > > > /* check to make sure log sector is legal 2^N, 9 <= N <= 15 */ > > @@ -494,6 +499,10 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) > > return(XR_BAD_SB_WIDTH); > > } > > > > + /* Directory block log */ > > + if (sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG) > > + return XR_BAD_FS_SIZE_DATA; > > + > > OK anyway, if any of this fails we go and take a vote from secondaries, > should be fine. > > > return(XR_OK); > > } > > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > index 5c79fd9..8d4be83 100644 > > --- a/repair/xfs_repair.c > > +++ b/repair/xfs_repair.c > > @@ -621,7 +621,10 @@ is_multidisk_filesystem( > > if (!sbp->sb_unit) > > return false; > > > > - ASSERT(sbp->sb_width); > > + /* Stripe unit but no stripe width? Something's funny here... */ > > + if (!sbp->sb_width) > > + return false; > > + > > verify_sb did already sanitize this... > > /* > * verify stripe alignment fields if present > */ > if (xfs_sb_version_hasdalign(sb)) { > if ((!sb->sb_unit && sb->sb_width) || > (sb->sb_unit && sb->sb_agblocks % sb->sb_unit)) > return(XR_BAD_SB_UNIT); > if ((sb->sb_unit && !sb->sb_width) || > (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit)) > return(XR_BAD_SB_WIDTH); > } > > so we do this when we start up: > > main > get_sb > verify_sb > ... > is_multidisk_filesystem > > by then it should have been ok, what path did you hit this on? > > Seems like we shouldn't be making decisions about "is multidisk" > here, we should be saying "bad superblock" somewhere earlier, no? > > I mean, bailing with false is fine I /guess/ but I think the ASSERT > implies that we should have already verified that both or none > are set... Start with a filesystem with sunit = swidth = 0 and !hasdalign. # xfs_db -x -c 'sb 0' -c 'fuzz -d sb_unit random' /dev/XXX Note that we /don't/ turn on XFS_SB_VERSION_DALIGNBIT here, so the verify_sb check doesn't reject the sb. But then is_multidisk_filesystem fails to look for hasdalign and just goes ahead and uses sunit/swidth, triggering the assert. --D > > -Eric > > > return true; > > } > > > > > > -- > > 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/repair/sb.c b/repair/sb.c index 004702c..d784420 100644 --- a/repair/sb.c +++ b/repair/sb.c @@ -395,21 +395,26 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) /* sanity check ag count, size fields against data size field */ if (sb->sb_dblocks == 0 || - sb->sb_dblocks > - ((__uint64_t)sb->sb_agcount * sb->sb_agblocks) || - sb->sb_dblocks < - ((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks - + XFS_MIN_AG_BLOCKS)) + sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) || + sb->sb_dblocks < XFS_MIN_DBLOCKS(sb)) return(XR_BAD_FS_SIZE_DATA); if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks)) return(XR_BAD_FS_SIZE_DATA); + if (sb->sb_inodelog < XFS_DINODE_MIN_LOG || + sb->sb_inodelog > XFS_DINODE_MAX_LOG || + sb->sb_inodelog != libxfs_log2_roundup(sb->sb_inodesize)) + return XR_BAD_INO_SIZE_DATA; + if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE || sb->sb_inodesize > XFS_DINODE_MAX_SIZE || sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize)) return(XR_BAD_INO_SIZE_DATA); + if (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) + return XR_BAD_INO_SIZE_DATA; + if (xfs_sb_version_hassector(sb)) { /* check to make sure log sector is legal 2^N, 9 <= N <= 15 */ @@ -494,6 +499,10 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) return(XR_BAD_SB_WIDTH); } + /* Directory block log */ + if (sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG) + return XR_BAD_FS_SIZE_DATA; + return(XR_OK); } diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c index 5c79fd9..8d4be83 100644 --- a/repair/xfs_repair.c +++ b/repair/xfs_repair.c @@ -621,7 +621,10 @@ is_multidisk_filesystem( if (!sbp->sb_unit) return false; - ASSERT(sbp->sb_width); + /* Stripe unit but no stripe width? Something's funny here... */ + if (!sbp->sb_width) + return false; + return true; }
In xfs_repair, the inodelog, sectlog, and dirblklog values are read directly into the xfs_mount structure without any sanity checking by the verifier. This results in xfs_repair segfaulting when those fields have ridiculously high values because the pointer arithmetic runs us off the end of the metadata buffers. Therefore, reject the superblock if these values are garbage and try to find one of the other ones. Also clean up the dblocks checking to use the relevant macros and remove a bogus ASSERT that crashes repair when sunit is set but swidth isn't. The superblock field fuzzer (xfs/1301) triggers all these segfaults. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- repair/sb.c | 19 ++++++++++++++----- repair/xfs_repair.c | 5 ++++- 2 files changed, 18 insertions(+), 6 deletions(-) -- 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