On Fri, Sep 04, 2020 at 04:25:15PM +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,
> 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@xxxxxxxxxx>
> ---
> v2: https://lore.kernel.org/r/20200902141923.26422-1-hsiangkao@xxxxxxxxxx
>
> 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()).
> + */
I wouldn't mention the exceptional case at all here since I think it
just adds confusion. It's an unfortunate wart with mkfs that requires a
kernel workaround, and I think it's better to keep it one place. I.e.,
should it ever be removed, I find it unlikely somebody will notice this
comment and fix it up accordingly.
> 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;
Please do something like the following so the full corruption check
logic is readable:
if (XFS_IS_CORRUPT(..., bufsize && hsize > bufsize))
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;
This technically defers broader corruption checks (i.e., magic number,
etc.) until after functional code starts using other fields below. I
don't think we should remove this.
>
> /*
> * 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);
I don't think we should update rhead like this, particularly in a rare
and exclusive case. This structure should reflect what is on disk.
All in all, I think this patch should be much more focused:
1.) Add the bufsize parameter and associated corruption check to
xlog_valid_rec_header().
2.) Pass the related value from the existing calls.
3.) (Optional) If there's reason to revalidate after executing the mkfs
workaround, add a second call within the branch that implements the
h_size workaround.
Also, please test the workaround case to make sure it still works as
expected (if you haven't already).
Brian
> }
>
> + 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
>
