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

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

[Brian Foster]

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

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

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

[Brian Foster]

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

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

[Dave Chinner]

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


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

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

Re: [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@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@fromorbit.com
> 
> _______________________________________________
> 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