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

Gao Xiang
2020-09-04T08:26:00Z

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()).
+ */
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;
+
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;

/*
* 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);
}

+ 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