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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
