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

[Gao Xiang <hsiangkao@redhat.com>]

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