On Tue, Jul 21, 2015 at 01:59:34PM -0400, Brian Foster wrote:
> On Tue, Jul 21, 2015 at 01:44:57PM -0400, Brian Foster wrote:
> > Each log record header contains an h_size value which represents the
> > size of the iclog buffers when the record was logged and an h_len value
> > which identifies the length of the particular log record. Log recovery
> > uses both fields to determine the size of the log buffers to use for
> > recovery and to correctly process each log record.
> >
> > Neither field is completely validated during recovery, however. While
> > on-disk corruptions might be detected by CRC verification, we are still
> > susceptible to errors such as excessively sized buffer allocations and
> > overruns of the log record header and data buffers.
> >
> > Update the xlog_valid_rec_header() function to validate the record
> > h_size and h_len fields against a new max_size parameter. The maximum
> > size value is passed as a parameter because the value differs depending
> > on whether we are trying to identify the iclog size or actually
> > processing a log record. In the former case, validate that neither field
> > exceeds the maximum supported iclog size of XFS. Once the iclog size is
> > identified and log record buffers allocated, validate all records to be
> > processed against the iclog size to ensure that buffer overrun cannot
> > occur.
> >
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> >
> > Here's a stab at addressing the log record field validation issues Dave
> > calls out here:
> >
> > http://oss.sgi.com/pipermail/xfs/2015-July/042557.html
> >
> > This is still undergoing some testing, but I've not hit any problems so
> > far...
> >
>
> ... aaaaannd sure enough I managed to hit an assert blow up within a
> couple minutes of sending this out. ;)
>
> kernel: XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line:
> 2005
>
> I'm not yet sure whether it is related. It seems somewhat strange if it
> is. I'll see if it's reproducible with and without these patches.
> Thoughts appreciated on this change in the meantime...
Any update on this assert failure?
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 01dd228..98c420a 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -4078,13 +4078,22 @@ xlog_unpack_data(
> > return 0;
> > }
> >
> > +/*
> > + * Sanity check a log record header. The caller provides the maximum iclog
> > size
> > + * and record length since validity depends on the context. For example,
> > the
> > + * first record is used to allocate buffers and thus is validated against
> > the
> > + * maximum supported iclog size. Subsequent records must be validated
> > against
> > + * the identified iclog size to avoid overflow of the record buffers.
> > + */
> > STATIC int
> > xlog_valid_rec_header(
> > struct xlog *log,
> > struct xlog_rec_header *rhead,
> > - xfs_daddr_t blkno)
> > + xfs_daddr_t blkno,
> > + int max_size)
> > {
> > int hlen;
> > + int hsize;
> >
> > if (unlikely(rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM))) {
> > XFS_ERROR_REPORT("xlog_valid_rec_header(1)",
> > @@ -4099,14 +4108,21 @@ xlog_valid_rec_header(
> > return -EIO;
> > }
> >
> > + hsize = be32_to_cpu(rhead->h_size);
be32_to_cpu() returns an unsigned value, so the hsize/hlen variables
can de declared as u32/uint32_t, and then
> > + if (unlikely(hsize <= 0 || hsize > max_size)) {
The comparison for < 0 can go away.
Also, I don't think that unlikely() provides any value here - it
makes the code harder to read, it's not a performance sensitive
path, and, IIRC, gcc already weights branches that end in a return
statement as "unlikely".
> > + xfs_warn(log->l_mp, "%s: invalid iclog size (%d).", __func__,
> > + hsize);
> > + return -EFSCORRUPTED;
> > + }
> > +
> > /* LR body must have data or it wouldn't have been written */
> > hlen = be32_to_cpu(rhead->h_len);
> > - if (unlikely( hlen <= 0 || hlen > INT_MAX )) {
> > + if (unlikely(hlen <= 0 || hlen > max_size)) {
> > XFS_ERROR_REPORT("xlog_valid_rec_header(2)",
> > XFS_ERRLEVEL_LOW, log->l_mp);
> > return -EFSCORRUPTED;
> > }
> > - if (unlikely( blkno > log->l_logBBsize || blkno > INT_MAX )) {
> > + if (unlikely(blkno > log->l_logBBsize || blkno > INT_MAX)) {
> > XFS_ERROR_REPORT("xlog_valid_rec_header(3)",
> > XFS_ERRLEVEL_LOW, log->l_mp);
> > return -EFSCORRUPTED;
> > @@ -4159,7 +4175,8 @@ xlog_do_recovery_pass(
> > goto bread_err1;
> >
> > rhead = (xlog_rec_header_t *)offset;
> > - error = xlog_valid_rec_header(log, rhead, tail_blk);
> > + error = xlog_valid_rec_header(log, rhead, tail_blk,
> > + XLOG_MAX_RECORD_BSIZE);
For v1 logs this is XLOG_BIG_RECORD_BSIZE, not
XLOG_MAX_RECORD_BSIZE.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|