[PATCH] xfs: validate iclog size and log record length in log recovery

[Brian Foster]

On Mon, Jul 27, 2015 at 11:13:06AM +1000, Dave Chinner wrote:
> 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 at redhat.com>
> > > ---
> > > 
> > > 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?
> 

I haven't been able to reproduce so far. That said, my log recovery
testing thus far has been limited by the umount hang addressed by the
EFI patches I posted towards the end of last week. FWIW, that series
alone underwent a decent amount of recovery testing to show that it at
least addressed the hang without reproducing any such assert failures.
I'll have to put these all together in a series and beat on that for a
while.

> 
> > > 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.
> 

Ok.

> 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".
> 

Alright. I'll fix up the rest of the function as such.

> > > +		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.
>

This is all within an 'if (xfs_sb_version_haslogv2(...))' block. h_size
(which the subsequent xlog_valid_rec_header() calls use) is fixed to
XLOG_BIG_RECORD_BSIZE in the else block.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david at fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs