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

[Brian Foster <bfoster@redhat.com>]

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

Brian

> Brian
> 
>  fs/xfs/xfs_log_recover.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> 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);
> +	if (unlikely(hsize <= 0 || hsize > max_size)) {
> +		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);
>  		if (error)
>  			goto bread_err1;
>  		h_size = be32_to_cpu(rhead->h_size);
> @@ -4244,7 +4261,8 @@ xlog_do_recovery_pass(
>  			}
>  			rhead = (xlog_rec_header_t *)offset;
>  			error = xlog_valid_rec_header(log, rhead,
> -						split_hblks ? blk_no : 0);
> +						      split_hblks ? blk_no : 0,
> +						      h_size);
>  			if (error)
>  				goto bread_err2;
>  
> @@ -4318,7 +4336,7 @@ xlog_do_recovery_pass(
>  			goto bread_err2;
>  
>  		rhead = (xlog_rec_header_t *)offset;
> -		error = xlog_valid_rec_header(log, rhead, blk_no);
> +		error = xlog_valid_rec_header(log, rhead, blk_no, h_size);
>  		if (error)
>  			goto bread_err2;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs