Re: [PATCH v3 1/2] xfs: avoid LR buffer overrun due to crafted h_{len,size}

[Brian Foster <bfoster@redhat.com>]

On Fri, Sep 04, 2020 at 04:25:15PM +0800, Gao Xiang wrote:
> Currently, crafted h_len has been blocked for the log
> header of the tail block in commit a70f9fe52daa ("xfs:
> detect and handle invalid iclog size set by mkfs").
> 
> However, each log record could still have crafted h_len,
> h_size and cause log record buffer overrun. So let's
> check (h_len vs h_size) and (h_size vs buffer size)
> for each log record as well instead.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> v2: https://lore.kernel.org/r/20200902141923.26422-1-hsiangkao@redhat.com
> 
> changes since v2:
>  - rename argument h_size to bufsize to make it clear (Brian);
>  - leave the mkfs workaround logic in xlog_do_recovery_pass() (Brian);
>  - add XLOG_VERSION_2 checking logic since old logrecv1 doesn't have
>    h_size field just to be safe.
> 
>  fs/xfs/xfs_log_recover.c | 50 +++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e2ec91b2d0f4..28d952794bfa 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2904,9 +2904,10 @@ STATIC int
>  xlog_valid_rec_header(
>  	struct xlog		*log,
>  	struct xlog_rec_header	*rhead,
> -	xfs_daddr_t		blkno)
> +	xfs_daddr_t		blkno,
> +	int			bufsize)
>  {
> -	int			hlen;
> +	int			hlen, hsize = XLOG_BIG_RECORD_BSIZE;
>  
>  	if (XFS_IS_CORRUPT(log->l_mp,
>  			   rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM)))
> @@ -2920,10 +2921,22 @@ xlog_valid_rec_header(
>  		return -EFSCORRUPTED;
>  	}
>  
> -	/* LR body must have data or it wouldn't have been written */
> +	/*
> +	 * LR body must have data (or it wouldn't have been written) and
> +	 * h_len must not be greater than h_size with one exception (see
> +	 * comments in xlog_do_recovery_pass()).
> +	 */

I wouldn't mention the exceptional case at all here since I think it
just adds confusion. It's an unfortunate wart with mkfs that requires a
kernel workaround, and I think it's better to keep it one place. I.e.,
should it ever be removed, I find it unlikely somebody will notice this
comment and fix it up accordingly.

>  	hlen = be32_to_cpu(rhead->h_len);
> -	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > INT_MAX))
> +	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb) &&
> +	    (be32_to_cpu(rhead->h_version) & XLOG_VERSION_2))
> +		hsize = be32_to_cpu(rhead->h_size);
> +
> +	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > hsize))
>  		return -EFSCORRUPTED;
> +
> +	if (bufsize && XFS_IS_CORRUPT(log->l_mp, bufsize < hsize))
> +		return -EFSCORRUPTED;

Please do something like the following so the full corruption check
logic is readable:

	if (XFS_IS_CORRUPT(..., bufsize && hsize > bufsize))
		return -EFSCORRUPTED;

> +
>  	if (XFS_IS_CORRUPT(log->l_mp,
>  			   blkno > log->l_logBBsize || blkno > INT_MAX))
>  		return -EFSCORRUPTED;
> @@ -2984,9 +2997,6 @@ xlog_do_recovery_pass(
>  			goto bread_err1;
>  
>  		rhead = (xlog_rec_header_t *)offset;
> -		error = xlog_valid_rec_header(log, rhead, tail_blk);
> -		if (error)
> -			goto bread_err1;

This technically defers broader corruption checks (i.e., magic number,
etc.) until after functional code starts using other fields below. I
don't think we should remove this.

>  
>  		/*
>  		 * xfsprogs has a bug where record length is based on lsunit but
> @@ -3001,21 +3011,19 @@ xlog_do_recovery_pass(
>  		 */
>  		h_size = be32_to_cpu(rhead->h_size);
>  		h_len = be32_to_cpu(rhead->h_len);
> -		if (h_len > h_size) {
> -			if (h_len <= log->l_mp->m_logbsize &&
> -			    be32_to_cpu(rhead->h_num_logops) == 1) {
> -				xfs_warn(log->l_mp,
> +		if (h_len > h_size && h_len <= log->l_mp->m_logbsize &&
> +		    rhead->h_num_logops == cpu_to_be32(1)) {
> +			xfs_warn(log->l_mp,
>  		"invalid iclog size (%d bytes), using lsunit (%d bytes)",
> -					 h_size, log->l_mp->m_logbsize);
> -				h_size = log->l_mp->m_logbsize;
> -			} else {
> -				XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> -						log->l_mp);
> -				error = -EFSCORRUPTED;
> -				goto bread_err1;
> -			}
> +				 h_size, log->l_mp->m_logbsize);
> +			h_size = log->l_mp->m_logbsize;
> +			rhead->h_size = cpu_to_be32(h_size);

I don't think we should update rhead like this, particularly in a rare
and exclusive case. This structure should reflect what is on disk.

All in all, I think this patch should be much more focused:

1.) Add the bufsize parameter and associated corruption check to
xlog_valid_rec_header().
2.) Pass the related value from the existing calls.
3.) (Optional) If there's reason to revalidate after executing the mkfs
workaround, add a second call within the branch that implements the
h_size workaround.

Also, please test the workaround case to make sure it still works as
expected (if you haven't already).

Brian

>  		}
>  
> +		error = xlog_valid_rec_header(log, rhead, tail_blk, 0);
> +		if (error)
> +			goto bread_err1;
> +
>  		if ((be32_to_cpu(rhead->h_version) & XLOG_VERSION_2) &&
>  		    (h_size > XLOG_HEADER_CYCLE_SIZE)) {
>  			hblks = h_size / XLOG_HEADER_CYCLE_SIZE;
> @@ -3096,7 +3104,7 @@ 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;
>  
> @@ -3177,7 +3185,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.18.1
>