| Message ID | 20171025185705.64983-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Oct 25, 2017 at 02:57:03PM -0400, Brian Foster wrote: > mkfs has a historical problem where it can format very small > filesystems with too small of a physical log. Under certain > conditions, log recovery of an associated filesystem can end up > passing garbage parameter values to some of the cycle and log record > verification functions due to bugs in log recovery not dealing with > such filesystems properly. This results in attempts to read from > bogus/underflowed log block addresses. > > Since the buffer read may ultimately succeed, log recovery can > proceed with bogus data and otherwise go off the rails and crash. > One example of this is a negative last_blk being passed to > xlog_find_verify_log_record() causing us to skip the loop, pass a > NULL head pointer to xlog_header_check_mount() and crash. > > Improve the xlog buffer verification to address this problem. We > already verify xlog buffer length, so update this mechanism to also > sanity check for a valid log relative block address and otherwise > return an error. Pass a fixed, valid log block address from > xlog_get_bp() since the target address will be validated when the > buffer is read. This ensures that any bogus log block address/length > calculations lead to graceful mount failure rather than risking a > crash or worse if recovery proceeds with bogus data. > > Reported-by: Zorro Lang <zlang@redhat.com> > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- > 1 file changed, 24 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index ee34899..54494ab 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -85,17 +85,21 @@ struct xfs_buf_cancel { > */ > > /* > - * Verify the given count of basic blocks is valid number of blocks > - * to specify for an operation involving the given XFS log buffer. > - * Returns nonzero if the count is valid, 0 otherwise. > + * Verify the log-relative block number and length in basic blocks are valid for > + * an operation involving the given XFS log buffer. Returns true if the fields > + * are valid, false otherwise. > */ > - > -static inline int > -xlog_buf_bbcount_valid( > +static inline bool > +xlog_verify_bp( > struct xlog *log, > + xfs_daddr_t blk_no, > int bbcount) > { > - return bbcount > 0 && bbcount <= log->l_logBBsize; > + if (blk_no < 0 || blk_no >= log->l_logBBsize) > + return false; > + if (bbcount <= 0 || bbcount > log->l_logBBsize) > + return false; Shouldn't this be (blk_no + bbcount) > log->l_logBBsize, since the blk_no/bbcount parameters identify an extent within the log? --D > + return true; > } > > /* > @@ -110,7 +114,11 @@ xlog_get_bp( > { > struct xfs_buf *bp; > > - if (!xlog_buf_bbcount_valid(log, nbblks)) { > + /* > + * Pass log block 0 since we don't have an addr yet, buffer will be > + * verified on read. > + */ > + if (!xlog_verify_bp(log, 0, nbblks)) { > xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", > nbblks); > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); > @@ -180,9 +188,10 @@ xlog_bread_noalign( > { > int error; > > - if (!xlog_buf_bbcount_valid(log, nbblks)) { > - xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", > - nbblks); > + if (!xlog_verify_bp(log, blk_no, nbblks)) { > + xfs_warn(log->l_mp, > + "Invalid log block/length (0x%llx, 0x%x) for buffer", > + blk_no, nbblks); > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); > return -EFSCORRUPTED; > } > @@ -265,9 +274,10 @@ xlog_bwrite( > { > int error; > > - if (!xlog_buf_bbcount_valid(log, nbblks)) { > - xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", > - nbblks); > + if (!xlog_verify_bp(log, blk_no, nbblks)) { > + xfs_warn(log->l_mp, > + "Invalid log block/length (0x%llx, 0x%x) for buffer", > + blk_no, nbblks); > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); > return -EFSCORRUPTED; > } > -- > 2.9.5 > > -- > 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 Wed, Oct 25, 2017 at 03:12:08PM -0700, Darrick J. Wong wrote: > On Wed, Oct 25, 2017 at 02:57:03PM -0400, Brian Foster wrote: > > mkfs has a historical problem where it can format very small > > filesystems with too small of a physical log. Under certain > > conditions, log recovery of an associated filesystem can end up > > passing garbage parameter values to some of the cycle and log record > > verification functions due to bugs in log recovery not dealing with > > such filesystems properly. This results in attempts to read from > > bogus/underflowed log block addresses. > > > > Since the buffer read may ultimately succeed, log recovery can > > proceed with bogus data and otherwise go off the rails and crash. > > One example of this is a negative last_blk being passed to > > xlog_find_verify_log_record() causing us to skip the loop, pass a > > NULL head pointer to xlog_header_check_mount() and crash. > > > > Improve the xlog buffer verification to address this problem. We > > already verify xlog buffer length, so update this mechanism to also > > sanity check for a valid log relative block address and otherwise > > return an error. Pass a fixed, valid log block address from > > xlog_get_bp() since the target address will be validated when the > > buffer is read. This ensures that any bogus log block address/length > > calculations lead to graceful mount failure rather than risking a > > crash or worse if recovery proceeds with bogus data. > > > > Reported-by: Zorro Lang <zlang@redhat.com> > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- > > 1 file changed, 24 insertions(+), 14 deletions(-) > > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > index ee34899..54494ab 100644 > > --- a/fs/xfs/xfs_log_recover.c > > +++ b/fs/xfs/xfs_log_recover.c > > @@ -85,17 +85,21 @@ struct xfs_buf_cancel { > > */ > > > > /* > > - * Verify the given count of basic blocks is valid number of blocks > > - * to specify for an operation involving the given XFS log buffer. > > - * Returns nonzero if the count is valid, 0 otherwise. > > + * Verify the log-relative block number and length in basic blocks are valid for > > + * an operation involving the given XFS log buffer. Returns true if the fields > > + * are valid, false otherwise. > > */ > > - > > -static inline int > > -xlog_buf_bbcount_valid( > > +static inline bool > > +xlog_verify_bp( > > struct xlog *log, > > + xfs_daddr_t blk_no, > > int bbcount) > > { > > - return bbcount > 0 && bbcount <= log->l_logBBsize; > > + if (blk_no < 0 || blk_no >= log->l_logBBsize) > > + return false; > > + if (bbcount <= 0 || bbcount > log->l_logBBsize) > > + return false; > > Shouldn't this be (blk_no + bbcount) > log->l_logBBsize, since the > blk_no/bbcount parameters identify an extent within the log? > Yes, I suppose we can do that since we pass blk_no = 0 from the get_bp() case. The new invocations pass the blockcount of the requested I/O operation (as opposed to the buffer size, which is probably what I was thinking), so that seems reasonable to me. Brian > --D > > > + return true; > > } > > > > /* > > @@ -110,7 +114,11 @@ xlog_get_bp( > > { > > struct xfs_buf *bp; > > > > - if (!xlog_buf_bbcount_valid(log, nbblks)) { > > + /* > > + * Pass log block 0 since we don't have an addr yet, buffer will be > > + * verified on read. > > + */ > > + if (!xlog_verify_bp(log, 0, nbblks)) { > > xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", > > nbblks); > > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); > > @@ -180,9 +188,10 @@ xlog_bread_noalign( > > { > > int error; > > > > - if (!xlog_buf_bbcount_valid(log, nbblks)) { > > - xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", > > - nbblks); > > + if (!xlog_verify_bp(log, blk_no, nbblks)) { > > + xfs_warn(log->l_mp, > > + "Invalid log block/length (0x%llx, 0x%x) for buffer", > > + blk_no, nbblks); > > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); > > return -EFSCORRUPTED; > > } > > @@ -265,9 +274,10 @@ xlog_bwrite( > > { > > int error; > > > > - if (!xlog_buf_bbcount_valid(log, nbblks)) { > > - xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", > > - nbblks); > > + if (!xlog_verify_bp(log, blk_no, nbblks)) { > > + xfs_warn(log->l_mp, > > + "Invalid log block/length (0x%llx, 0x%x) for buffer", > > + blk_no, nbblks); > > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); > > return -EFSCORRUPTED; > > } > > -- > > 2.9.5 > > > > -- > > 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/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index ee34899..54494ab 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -85,17 +85,21 @@ struct xfs_buf_cancel { */ /* - * Verify the given count of basic blocks is valid number of blocks - * to specify for an operation involving the given XFS log buffer. - * Returns nonzero if the count is valid, 0 otherwise. + * Verify the log-relative block number and length in basic blocks are valid for + * an operation involving the given XFS log buffer. Returns true if the fields + * are valid, false otherwise. */ - -static inline int -xlog_buf_bbcount_valid( +static inline bool +xlog_verify_bp( struct xlog *log, + xfs_daddr_t blk_no, int bbcount) { - return bbcount > 0 && bbcount <= log->l_logBBsize; + if (blk_no < 0 || blk_no >= log->l_logBBsize) + return false; + if (bbcount <= 0 || bbcount > log->l_logBBsize) + return false; + return true; } /* @@ -110,7 +114,11 @@ xlog_get_bp( { struct xfs_buf *bp; - if (!xlog_buf_bbcount_valid(log, nbblks)) { + /* + * Pass log block 0 since we don't have an addr yet, buffer will be + * verified on read. + */ + if (!xlog_verify_bp(log, 0, nbblks)) { xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", nbblks); XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); @@ -180,9 +188,10 @@ xlog_bread_noalign( { int error; - if (!xlog_buf_bbcount_valid(log, nbblks)) { - xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", - nbblks); + if (!xlog_verify_bp(log, blk_no, nbblks)) { + xfs_warn(log->l_mp, + "Invalid log block/length (0x%llx, 0x%x) for buffer", + blk_no, nbblks); XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); return -EFSCORRUPTED; } @@ -265,9 +274,10 @@ xlog_bwrite( { int error; - if (!xlog_buf_bbcount_valid(log, nbblks)) { - xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer", - nbblks); + if (!xlog_verify_bp(log, blk_no, nbblks)) { + xfs_warn(log->l_mp, + "Invalid log block/length (0x%llx, 0x%x) for buffer", + blk_no, nbblks); XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp); return -EFSCORRUPTED; }
mkfs has a historical problem where it can format very small filesystems with too small of a physical log. Under certain conditions, log recovery of an associated filesystem can end up passing garbage parameter values to some of the cycle and log record verification functions due to bugs in log recovery not dealing with such filesystems properly. This results in attempts to read from bogus/underflowed log block addresses. Since the buffer read may ultimately succeed, log recovery can proceed with bogus data and otherwise go off the rails and crash. One example of this is a negative last_blk being passed to xlog_find_verify_log_record() causing us to skip the loop, pass a NULL head pointer to xlog_header_check_mount() and crash. Improve the xlog buffer verification to address this problem. We already verify xlog buffer length, so update this mechanism to also sanity check for a valid log relative block address and otherwise return an error. Pass a fixed, valid log block address from xlog_get_bp() since the target address will be validated when the buffer is read. This ensures that any bogus log block address/length calculations lead to graceful mount failure rather than risking a crash or worse if recovery proceeds with bogus data. Reported-by: Zorro Lang <zlang@redhat.com> Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)