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@xxxxxxxxxx>
/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@xxxxxxxxxx>
--D
> ---
> v3: https://lore.kernel.org/r/20200904082516.31205-2-hsiangkao@xxxxxxxxxx
>
> 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@xxxxxxxxxx
> 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@xxxxxxxxxx
> 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
>
