Re: [PATCH v4 1/2] xfs: avoid LR buffer overrun due to crafted h_len

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Sep 17, 2020 at 01:13:40PM +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
> and cause log record buffer overrun. So let's check
> h_len vs buffer size for each log record as well.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

/me squints real hard and thinks he understands what this patch does.

Tighten up xlog_valid_rec_header, and add a new callsite in the middle
of xlog_do_recovery_pass instead of the insufficient length checking.
Assuming that's right,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> v3: https://lore.kernel.org/r/20200904082516.31205-2-hsiangkao@redhat.com
> 
> changes since v3:
>  - drop exception comment to avoid confusion (Brian);
>  - check rhead->hlen vs buffer size to address
>    the harmful overflow (Brian);
> 
> And as Brian requested previously, "Also, please test the workaround
> case to make sure it still works as expected (if you haven't already)."
> 
> So I tested the vanilla/after upstream kernels with compiled xfsprogs-v4.3.0,
> which was before mkfs fix 20fbd4593ff2 ("libxfs: format the log with
> valid log record headers") got merged, and I generated a questionable
> image followed by the instruction described in the related commit
> messages with "mkfs.xfs -dsunit=512,swidth=4096 -f test.img" and
> logprint says
> 
> cycle: 1        version: 2              lsn: 1,0        tail_lsn: 1,0
> length of Log Record: 261632    prev offset: -1         num ops: 1
> uuid: 7b84cd80-7855-4dc8-91ce-542c7d65ba99   format: little endian linux
> h_size: 32768
> 
> so "length of Log Record" is overrun obviously, but I cannot reproduce
> the described workaround case for vanilla/after kernels anymore.
> 
> I think the reason is due to commit 7f6aff3a29b0 ("xfs: only run torn
> log write detection on dirty logs"), which changes the behavior
> described in commit a70f9fe52daa8 ("xfs: detect and handle invalid
> iclog size set by mkfs") from "all records at the head of the log
> are verified for CRC errors" to "we can only run CRC verification
> when the log is dirty because there's no guarantee that the log
> data behind an unmount record is compatible with the current
> architecture).", more details see codediff of a70f9fe52daa8.
> 
> The timeline seems to be:
>  https://lore.kernel.org/r/1447342648-40012-1-git-send-email-bfoster@redhat.com
>  a70f9fe52daa [PATCH v2 1/8] xfs: detect and handle invalid iclog size set by mkfs
>  7088c4136fa1 [PATCH v2 7/8] xfs: detect and trim torn writes during log recovery
>  https://lore.kernel.org/r/1457008798-58734-5-git-send-email-bfoster@redhat.com
>  7f6aff3a29b0 [PATCH 4/4] xfs: only run torn log write detection on dirty logs
> 
> so IMHO, the workaround a70f9fe52daa would only be useful between
> 7088c4136fa1 ~ 7f6aff3a29b0.
> 
> Yeah, it relates to several old kernel commits/versions, my technical
> analysis is as above. This patch doesn't actually change the real
> original workaround logic. Even if the workground can be removed now,
> that should be addressed with another patch and that is quite another
> story.
> 
> Kindly correct me if I'm wrong :-)
> 
> Thanks,
> Gao Xiang
> 
>  fs/xfs/xfs_log_recover.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index a17d788921d6..782ec3eeab4d 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2878,7 +2878,8 @@ 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;
>  
> @@ -2894,10 +2895,14 @@ 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 LR buffer size.
> +	 */
>  	hlen = be32_to_cpu(rhead->h_len);
> -	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > INT_MAX))
> +	if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > bufsize))
>  		return -EFSCORRUPTED;
> +
>  	if (XFS_IS_CORRUPT(log->l_mp,
>  			   blkno > log->l_logBBsize || blkno > INT_MAX))
>  		return -EFSCORRUPTED;
> @@ -2958,9 +2963,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;
>  
>  		/*
>  		 * xfsprogs has a bug where record length is based on lsunit but
> @@ -2975,21 +2977,18 @@ 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;
>  		}
>  
> +		error = xlog_valid_rec_header(log, rhead, tail_blk, h_size);
> +		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;
> @@ -3070,7 +3069,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;
>  
> @@ -3151,7 +3150,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
>