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

[Gao Xiang <hsiangkao@redhat.com>]

Hi Brian,

On Fri, Sep 04, 2020 at 07:25:29AM -0400, Brian Foster wrote:
> On Fri, Sep 04, 2020 at 04:25:15PM +0800, Gao Xiang wrote:

...

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

Thanks for your review.

ok, if I understand correctly, will remove this "with one exception
(see comments..." expression. Please kindly correct me if I
misunderstand.

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

That is good idea, will update this as well. 

>

...

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

I'm trying to combine this with the following part...(see below...)

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

I moved workaround code to xlog_valid_rec_header() at first is
because in xlog_valid_rec_header() actually it has 2 individual
checks now:

1) check rhead->h_len vs rhead->h_size for each individual log record;
2) check rhead->h_size vs the unique allocated buffer size passed in
   for each record (since each log record has one stored h_size,
   even though there are not used later according to the current
   logic of xlog_do_recovery_pass).

if any of the conditions above is not satisfied, xlog_valid_rec_header()
will make fs corrupted immediately, so I tried 2 ways up to now:

 - (v1,v2) fold in workaround case into xlog_valid_rec_header()
 - (v3) rearrange workaround and xlog_valid_rec_header() order in
        xlog_do_recovery_pass() and modify rhead->h_size to the
        workaround h_size before xlog_valid_rec_header() validation
        so xlog_valid_rec_header() will work as expected since it
        has two individual checks as mentioned above.

If there is some better way, kindly let me know :) and I'd like to
hear other folks about this in advance as well.... so I can go
forward since this part is a bit tricky for now.

> Also, please test the workaround case to make sure it still works as
> expected (if you haven't already).

ok, will double confirm this, thanks!

Thanks,
Gao Xiang

> 
> Brian
>