[PATCH v2 0/2] xfs: random patches on log recovery

[PATCH v2 0/2] xfs: random patches on log recovery

[Gao Xiang]

Hi folks,

Here are some patches after I read recovery code days ago.
Due to code coupling, I send them as a patchset.

This version mainly addresses previous review from Brian,
sorry for taking some time due to another work.

I already ran fstests and no strange out on my side and
detail changelog is in each individual patch.

Thanks,
Gao Xiang

Gao Xiang (2):
  xfs: avoid LR buffer overrun due to crafted h_len
  xfs: clean up calculation of LR header blocks

 fs/xfs/xfs_log.c         |  4 +-
 fs/xfs/xfs_log_recover.c | 87 ++++++++++++++++------------------------
 2 files changed, 36 insertions(+), 55 deletions(-)

-- 
2.18.1

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

[Gao Xiang]

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

[PATCH v3 2/2] xfs: clean up calculation of LR header blocks

[Gao Xiang]

Let's use DIV_ROUND_UP() to calculate log record header
blocks as what did in xlog_get_iclog_buffer_size() and
wrap up a common helper for log recovery.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
v2: https://lore.kernel.org/r/20200904082516.31205-3-hsiangkao@redhat.com

changes since v2:
 - get rid of xlog_logrecv2_hblks() and use xlog_logrec_hblks()
   entirely (Brian).

 fs/xfs/xfs_log.c         |  4 +---
 fs/xfs/xfs_log_recover.c | 48 ++++++++++++++--------------------------
 2 files changed, 17 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index ad0c69ee8947..7a4ba408a3a2 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1604,9 +1604,7 @@ xlog_cksum(
 		int		i;
 		int		xheads;
 
-		xheads = size / XLOG_HEADER_CYCLE_SIZE;
-		if (size % XLOG_HEADER_CYCLE_SIZE)
-			xheads++;
+		xheads = DIV_ROUND_UP(size, XLOG_HEADER_CYCLE_SIZE);
 
 		for (i = 1; i < xheads; i++) {
 			crc = crc32c(crc, &xhdr[i].hic_xheader,
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 782ec3eeab4d..28dd98b5a703 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -371,6 +371,18 @@ xlog_find_verify_cycle(
 	return error;
 }
 
+static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh)
+{
+	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
+		int	h_size = be32_to_cpu(rh->h_size);
+
+		if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) &&
+		    h_size > XLOG_HEADER_CYCLE_SIZE)
+			return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
+	}
+	return 1;
+}
+
 /*
  * Potentially backup over partial log record write.
  *
@@ -463,15 +475,7 @@ xlog_find_verify_log_record(
 	 * reset last_blk.  Only when last_blk points in the middle of a log
 	 * record do we update last_blk.
 	 */
-	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
-		uint	h_size = be32_to_cpu(head->h_size);
-
-		xhdrs = h_size / XLOG_HEADER_CYCLE_SIZE;
-		if (h_size % XLOG_HEADER_CYCLE_SIZE)
-			xhdrs++;
-	} else {
-		xhdrs = 1;
-	}
+	xhdrs = xlog_logrec_hblks(log, head);
 
 	if (*last_blk - i + extra_bblks !=
 	    BTOBB(be32_to_cpu(head->h_len)) + xhdrs)
@@ -1158,22 +1162,7 @@ xlog_check_unmount_rec(
 	 * below. We won't want to clear the unmount record if there is one, so
 	 * we pass the lsn of the unmount record rather than the block after it.
 	 */
-	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
-		int	h_size = be32_to_cpu(rhead->h_size);
-		int	h_version = be32_to_cpu(rhead->h_version);
-
-		if ((h_version & XLOG_VERSION_2) &&
-		    (h_size > XLOG_HEADER_CYCLE_SIZE)) {
-			hblks = h_size / XLOG_HEADER_CYCLE_SIZE;
-			if (h_size % XLOG_HEADER_CYCLE_SIZE)
-				hblks++;
-		} else {
-			hblks = 1;
-		}
-	} else {
-		hblks = 1;
-	}
-
+	hblks = xlog_logrec_hblks(log, rhead);
 	after_umount_blk = xlog_wrap_logbno(log,
 			rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len)));
 
@@ -2989,15 +2978,10 @@ xlog_do_recovery_pass(
 		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;
-			if (h_size % XLOG_HEADER_CYCLE_SIZE)
-				hblks++;
+		hblks = xlog_logrec_hblks(log, rhead);
+		if (hblks != 1) {
 			kmem_free(hbp);
 			hbp = xlog_alloc_buffer(log, hblks);
-		} else {
-			hblks = 1;
 		}
 	} else {
 		ASSERT(log->l_sectBBsize == 1);
-- 
2.18.1

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

[Brian Foster]

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

If I follow correctly, you're saying that prior to commit 7f6aff3a29b0,
log recovery would run a CRC pass on a clean log (with an unmount
record) and this is where the old workaround code would kick in if the
filesystem happened to be misformatted by mkfs. After said commit, the
CRC pass is no longer run unless the log is dirty (for arch
compatibility reasons), so we fall into the xlog_check_unmount_rec()
path that does some careful (presumably arch agnostic) detection of a
clean/dirty log based on whether the record just behind the head has a
single unmount transaction. This function already uses h_len properly
and only reads a single log block to determine whether the target is an
unmount record, so doesn't have the same overflow risk as a full
recovery pass.

Am I following that correctly? If so, the patch otherwise looks
reasonable to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

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

Re: [PATCH v3 2/2] xfs: clean up calculation of LR header blocks

[Brian Foster]

On Thu, Sep 17, 2020 at 01:13:41PM +0800, Gao Xiang wrote:
> Let's use DIV_ROUND_UP() to calculate log record header
> blocks as what did in xlog_get_iclog_buffer_size() and
> wrap up a common helper for log recovery.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> v2: https://lore.kernel.org/r/20200904082516.31205-3-hsiangkao@redhat.com
> 
> changes since v2:
>  - get rid of xlog_logrecv2_hblks() and use xlog_logrec_hblks()
>    entirely (Brian).
> 
>  fs/xfs/xfs_log.c         |  4 +---
>  fs/xfs/xfs_log_recover.c | 48 ++++++++++++++--------------------------
>  2 files changed, 17 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index ad0c69ee8947..7a4ba408a3a2 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1604,9 +1604,7 @@ xlog_cksum(
>  		int		i;
>  		int		xheads;
>  
> -		xheads = size / XLOG_HEADER_CYCLE_SIZE;
> -		if (size % XLOG_HEADER_CYCLE_SIZE)
> -			xheads++;
> +		xheads = DIV_ROUND_UP(size, XLOG_HEADER_CYCLE_SIZE);
>  
>  		for (i = 1; i < xheads; i++) {
>  			crc = crc32c(crc, &xhdr[i].hic_xheader,
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 782ec3eeab4d..28dd98b5a703 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -371,6 +371,18 @@ xlog_find_verify_cycle(
>  	return error;
>  }
>  
> +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh)
> +{

We're trying to gradually eliminate various structure typedefs so it's
frowned upon to introduce new instances. If you replace the above with
'struct xlog_rec_header,' the rest looks good to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> +		int	h_size = be32_to_cpu(rh->h_size);
> +
> +		if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) &&
> +		    h_size > XLOG_HEADER_CYCLE_SIZE)
> +			return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
> +	}
> +	return 1;
> +}
> +
>  /*
>   * Potentially backup over partial log record write.
>   *
> @@ -463,15 +475,7 @@ xlog_find_verify_log_record(
>  	 * reset last_blk.  Only when last_blk points in the middle of a log
>  	 * record do we update last_blk.
>  	 */
> -	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> -		uint	h_size = be32_to_cpu(head->h_size);
> -
> -		xhdrs = h_size / XLOG_HEADER_CYCLE_SIZE;
> -		if (h_size % XLOG_HEADER_CYCLE_SIZE)
> -			xhdrs++;
> -	} else {
> -		xhdrs = 1;
> -	}
> +	xhdrs = xlog_logrec_hblks(log, head);
>  
>  	if (*last_blk - i + extra_bblks !=
>  	    BTOBB(be32_to_cpu(head->h_len)) + xhdrs)
> @@ -1158,22 +1162,7 @@ xlog_check_unmount_rec(
>  	 * below. We won't want to clear the unmount record if there is one, so
>  	 * we pass the lsn of the unmount record rather than the block after it.
>  	 */
> -	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> -		int	h_size = be32_to_cpu(rhead->h_size);
> -		int	h_version = be32_to_cpu(rhead->h_version);
> -
> -		if ((h_version & XLOG_VERSION_2) &&
> -		    (h_size > XLOG_HEADER_CYCLE_SIZE)) {
> -			hblks = h_size / XLOG_HEADER_CYCLE_SIZE;
> -			if (h_size % XLOG_HEADER_CYCLE_SIZE)
> -				hblks++;
> -		} else {
> -			hblks = 1;
> -		}
> -	} else {
> -		hblks = 1;
> -	}
> -
> +	hblks = xlog_logrec_hblks(log, rhead);
>  	after_umount_blk = xlog_wrap_logbno(log,
>  			rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len)));
>  
> @@ -2989,15 +2978,10 @@ xlog_do_recovery_pass(
>  		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;
> -			if (h_size % XLOG_HEADER_CYCLE_SIZE)
> -				hblks++;
> +		hblks = xlog_logrec_hblks(log, rhead);
> +		if (hblks != 1) {
>  			kmem_free(hbp);
>  			hbp = xlog_alloc_buffer(log, hblks);
> -		} else {
> -			hblks = 1;
>  		}
>  	} else {
>  		ASSERT(log->l_sectBBsize == 1);
> -- 
> 2.18.1
> 

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

[Gao Xiang]

Hi Brian,

On Tue, Sep 22, 2020 at 11:22:12AM -0400, Brian Foster wrote:
> 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>
> > ---
> > 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.
> > 
> 
> If I follow correctly, you're saying that prior to commit 7f6aff3a29b0,
> log recovery would run a CRC pass on a clean log (with an unmount
> record) and this is where the old workaround code would kick in if the
> filesystem happened to be misformatted by mkfs. After said commit, the
> CRC pass is no longer run unless the log is dirty (for arch
> compatibility reasons), so we fall into the xlog_check_unmount_rec()
> path that does some careful (presumably arch agnostic) detection of a
> clean/dirty log based on whether the record just behind the head has a
> single unmount transaction. This function already uses h_len properly
> and only reads a single log block to determine whether the target is an
> unmount record, so doesn't have the same overflow risk as a full
> recovery pass.
> 
> Am I following that correctly? If so, the patch otherwise looks
> reasonable to me:

Yeah, that is what I was trying to say. Thanks for the review!

Thanks,
Gao Xiang

> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > 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

Re: [PATCH v3 2/2] xfs: clean up calculation of LR header blocks

[Gao Xiang]

Hi Brian,

On Tue, Sep 22, 2020 at 11:22:48AM -0400, Brian Foster wrote:
> On Thu, Sep 17, 2020 at 01:13:41PM +0800, Gao Xiang wrote:
> > Let's use DIV_ROUND_UP() to calculate log record header
> > blocks as what did in xlog_get_iclog_buffer_size() and
> > wrap up a common helper for log recovery.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> > v2: https://lore.kernel.org/r/20200904082516.31205-3-hsiangkao@redhat.com
> > 
> > changes since v2:
> >  - get rid of xlog_logrecv2_hblks() and use xlog_logrec_hblks()
> >    entirely (Brian).
> > 
> >  fs/xfs/xfs_log.c         |  4 +---
> >  fs/xfs/xfs_log_recover.c | 48 ++++++++++++++--------------------------
> >  2 files changed, 17 insertions(+), 35 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index ad0c69ee8947..7a4ba408a3a2 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -1604,9 +1604,7 @@ xlog_cksum(
> >  		int		i;
> >  		int		xheads;
> >  
> > -		xheads = size / XLOG_HEADER_CYCLE_SIZE;
> > -		if (size % XLOG_HEADER_CYCLE_SIZE)
> > -			xheads++;
> > +		xheads = DIV_ROUND_UP(size, XLOG_HEADER_CYCLE_SIZE);
> >  
> >  		for (i = 1; i < xheads; i++) {
> >  			crc = crc32c(crc, &xhdr[i].hic_xheader,
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 782ec3eeab4d..28dd98b5a703 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -371,6 +371,18 @@ xlog_find_verify_cycle(
> >  	return error;
> >  }
> >  
> > +static inline int xlog_logrec_hblks(struct xlog *log, xlog_rec_header_t *rh)
> > +{
> 
> We're trying to gradually eliminate various structure typedefs so it's
> frowned upon to introduce new instances. If you replace the above with
> 'struct xlog_rec_header,' the rest looks good to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks for your review. Yeah, I noticed the eliminate movement
and I used xlog_rec_header_t by mistake.

I will resend a follow-up "in-reply-to" patch to fix that.

Thanks,
Gao Xiang

[PATCH v3.2] xfs: clean up calculation of LR header blocks

[Gao Xiang]

Let's use DIV_ROUND_UP() to calculate log record header
blocks as what did in xlog_get_iclog_buffer_size() and
wrap up a common helper for log recovery.

Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changelog:
 - 'xlog_rec_header_t' => 'struct xlog_rec_header' to eliminate
   various structure typedefs (Brian).

 fs/xfs/xfs_log.c         |  4 +---
 fs/xfs/xfs_log_recover.c | 49 ++++++++++++++--------------------------
 2 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index ad0c69ee8947..7a4ba408a3a2 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1604,9 +1604,7 @@ xlog_cksum(
 		int		i;
 		int		xheads;
 
-		xheads = size / XLOG_HEADER_CYCLE_SIZE;
-		if (size % XLOG_HEADER_CYCLE_SIZE)
-			xheads++;
+		xheads = DIV_ROUND_UP(size, XLOG_HEADER_CYCLE_SIZE);
 
 		for (i = 1; i < xheads; i++) {
 			crc = crc32c(crc, &xhdr[i].hic_xheader,
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 782ec3eeab4d..12cde89c090b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -371,6 +371,19 @@ xlog_find_verify_cycle(
 	return error;
 }
 
+static inline int
+xlog_logrec_hblks(struct xlog *log, struct xlog_rec_header *rh)
+{
+	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
+		int	h_size = be32_to_cpu(rh->h_size);
+
+		if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) &&
+		    h_size > XLOG_HEADER_CYCLE_SIZE)
+			return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
+	}
+	return 1;
+}
+
 /*
  * Potentially backup over partial log record write.
  *
@@ -463,15 +476,7 @@ xlog_find_verify_log_record(
 	 * reset last_blk.  Only when last_blk points in the middle of a log
 	 * record do we update last_blk.
 	 */
-	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
-		uint	h_size = be32_to_cpu(head->h_size);
-
-		xhdrs = h_size / XLOG_HEADER_CYCLE_SIZE;
-		if (h_size % XLOG_HEADER_CYCLE_SIZE)
-			xhdrs++;
-	} else {
-		xhdrs = 1;
-	}
+	xhdrs = xlog_logrec_hblks(log, head);
 
 	if (*last_blk - i + extra_bblks !=
 	    BTOBB(be32_to_cpu(head->h_len)) + xhdrs)
@@ -1158,22 +1163,7 @@ xlog_check_unmount_rec(
 	 * below. We won't want to clear the unmount record if there is one, so
 	 * we pass the lsn of the unmount record rather than the block after it.
 	 */
-	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
-		int	h_size = be32_to_cpu(rhead->h_size);
-		int	h_version = be32_to_cpu(rhead->h_version);
-
-		if ((h_version & XLOG_VERSION_2) &&
-		    (h_size > XLOG_HEADER_CYCLE_SIZE)) {
-			hblks = h_size / XLOG_HEADER_CYCLE_SIZE;
-			if (h_size % XLOG_HEADER_CYCLE_SIZE)
-				hblks++;
-		} else {
-			hblks = 1;
-		}
-	} else {
-		hblks = 1;
-	}
-
+	hblks = xlog_logrec_hblks(log, rhead);
 	after_umount_blk = xlog_wrap_logbno(log,
 			rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len)));
 
@@ -2989,15 +2979,10 @@ xlog_do_recovery_pass(
 		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;
-			if (h_size % XLOG_HEADER_CYCLE_SIZE)
-				hblks++;
+		hblks = xlog_logrec_hblks(log, rhead);
+		if (hblks != 1) {
 			kmem_free(hbp);
 			hbp = xlog_alloc_buffer(log, hblks);
-		} else {
-			hblks = 1;
 		}
 	} else {
 		ASSERT(log->l_sectBBsize == 1);
-- 
2.18.1

Re: [PATCH v3.2] xfs: clean up calculation of LR header blocks

[Darrick J. Wong]

On Tue, Sep 22, 2020 at 11:53:16PM +0800, Gao Xiang wrote:
> Let's use DIV_ROUND_UP() to calculate log record header
> blocks as what did in xlog_get_iclog_buffer_size() and
> wrap up a common helper for log recovery.
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

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

--D

> ---
> changelog:
>  - 'xlog_rec_header_t' => 'struct xlog_rec_header' to eliminate
>    various structure typedefs (Brian).
> 
>  fs/xfs/xfs_log.c         |  4 +---
>  fs/xfs/xfs_log_recover.c | 49 ++++++++++++++--------------------------
>  2 files changed, 18 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index ad0c69ee8947..7a4ba408a3a2 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1604,9 +1604,7 @@ xlog_cksum(
>  		int		i;
>  		int		xheads;
>  
> -		xheads = size / XLOG_HEADER_CYCLE_SIZE;
> -		if (size % XLOG_HEADER_CYCLE_SIZE)
> -			xheads++;
> +		xheads = DIV_ROUND_UP(size, XLOG_HEADER_CYCLE_SIZE);
>  
>  		for (i = 1; i < xheads; i++) {
>  			crc = crc32c(crc, &xhdr[i].hic_xheader,
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 782ec3eeab4d..12cde89c090b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -371,6 +371,19 @@ xlog_find_verify_cycle(
>  	return error;
>  }
>  
> +static inline int
> +xlog_logrec_hblks(struct xlog *log, struct xlog_rec_header *rh)
> +{
> +	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> +		int	h_size = be32_to_cpu(rh->h_size);
> +
> +		if ((be32_to_cpu(rh->h_version) & XLOG_VERSION_2) &&
> +		    h_size > XLOG_HEADER_CYCLE_SIZE)
> +			return DIV_ROUND_UP(h_size, XLOG_HEADER_CYCLE_SIZE);
> +	}
> +	return 1;
> +}
> +
>  /*
>   * Potentially backup over partial log record write.
>   *
> @@ -463,15 +476,7 @@ xlog_find_verify_log_record(
>  	 * reset last_blk.  Only when last_blk points in the middle of a log
>  	 * record do we update last_blk.
>  	 */
> -	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> -		uint	h_size = be32_to_cpu(head->h_size);
> -
> -		xhdrs = h_size / XLOG_HEADER_CYCLE_SIZE;
> -		if (h_size % XLOG_HEADER_CYCLE_SIZE)
> -			xhdrs++;
> -	} else {
> -		xhdrs = 1;
> -	}
> +	xhdrs = xlog_logrec_hblks(log, head);
>  
>  	if (*last_blk - i + extra_bblks !=
>  	    BTOBB(be32_to_cpu(head->h_len)) + xhdrs)
> @@ -1158,22 +1163,7 @@ xlog_check_unmount_rec(
>  	 * below. We won't want to clear the unmount record if there is one, so
>  	 * we pass the lsn of the unmount record rather than the block after it.
>  	 */
> -	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
> -		int	h_size = be32_to_cpu(rhead->h_size);
> -		int	h_version = be32_to_cpu(rhead->h_version);
> -
> -		if ((h_version & XLOG_VERSION_2) &&
> -		    (h_size > XLOG_HEADER_CYCLE_SIZE)) {
> -			hblks = h_size / XLOG_HEADER_CYCLE_SIZE;
> -			if (h_size % XLOG_HEADER_CYCLE_SIZE)
> -				hblks++;
> -		} else {
> -			hblks = 1;
> -		}
> -	} else {
> -		hblks = 1;
> -	}
> -
> +	hblks = xlog_logrec_hblks(log, rhead);
>  	after_umount_blk = xlog_wrap_logbno(log,
>  			rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len)));
>  
> @@ -2989,15 +2979,10 @@ xlog_do_recovery_pass(
>  		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;
> -			if (h_size % XLOG_HEADER_CYCLE_SIZE)
> -				hblks++;
> +		hblks = xlog_logrec_hblks(log, rhead);
> +		if (hblks != 1) {
>  			kmem_free(hbp);
>  			hbp = xlog_alloc_buffer(log, hblks);
> -		} else {
> -			hblks = 1;
>  		}
>  	} else {
>  		ASSERT(log->l_sectBBsize == 1);
> -- 
> 2.18.1
> 

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

[Darrick J. Wong]

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
> 

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

[Gao Xiang]

On Wed, Sep 23, 2020 at 09:35:40AM -0700, Darrick J. Wong wrote:
> 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>

Thanks for the review!

The main point is to check each individual LR h_len against LR buffer
size allocated first in xlog_do_recovery_pass() to avoid buffer
overflow triggered by CRC calc or follow-up steps (details in
https://lore.kernel.org/linux-xfs/20200902224726.GB4671@xiangao.remote.csb/ ).

no new callsite (just move the callsite after original workaround
code). Anyway, that is just a buffer overflow issue can be triggered
by corrupted log. Just a minor stuff but security guyes could also
keep eyes on such thing. I think that is right. :)

Thanks,
Gao Xiang

> 
> --D