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

[Brian Foster <bfoster@redhat.com>]

On Fri, Sep 04, 2020 at 11:01:59PM +0800, Gao Xiang wrote:
> On Fri, Sep 04, 2020 at 09:37:04AM -0400, Brian Foster wrote:
> > On Fri, Sep 04, 2020 at 08:46:34PM +0800, Gao Xiang wrote:
> ...
> > > > > @@ -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.
> > > 
> > 
> > My suggestion is to, at minimum, separate the above two logic changes
> > into separate patches. Item #2 above is a functional check in that it
> > ensures each record fits into the record buffer size we've allocated
> > (based on h_size) at the start of recovery. This item is what my
> > feedback above was referring to and I think is a fairly straightforward
> > change.
> 
> There are 3-value relationships now (rhead->h_len <= rhead->h_size <=
> unique allocated buffer size [which is the same as rhead->h_size of
> initial tail log record]).
> 
> The only buffer overrun case which could do harm runtimely is
> rhead->h_len > bufsize as I mentioned in the previous email
> https://lore.kernel.org/r/20200902224726.GB4671@xiangao.remote.csb
> 
> so I guess what you meant is
> 
>  - get h_size from the tail initial log record as buffer size (so
>    workaround needs to be settled here as well);
>  - call in xlog_valid_rec_header() to fail out rhead->h_len >
>    buffer size.
> 
> as the 1st splited patch. Yet that's not the case of Item #2
> (rhead->h_size <= buffer size), just to confirm if my understanding
> is correct.
> 

Yep.

> > 
> > Item #1 is a bit more nebulous. h_size refers to the iclog size used by
> > the kernel that wrote to the log. I think it should be uniform across
> > all records and AFAICT it doesn't serve a functional purpose (validation
> > notwithstanding) for recovery beyond establishing the size of the buffer
> > we should allocate to process records. The mkfs workaround implements a
> > case where we have to ignore the on-disk h_size and use the in-core
> > iclog size, and that is currently isolated to a single place. I'm not
> > fundamentally against more h_size verification beyond current usage, but
> > if that means we have to consider the workaround for multiple records
> > (which is confusing and incorrect) or make subtle runtime changes like
> > quietly tweaking the in-core record header structure from what is on
> > disk, then I don't think it's worth the complexity.
> > 
> > If we _really_ wanted to include such a change, I think a more
> > appropriate validation check might be to similarly track the h_size from
> > the initial record and simply verify that the values are uniform across
> > all processed records. That way we don't conflict or impose the
> > workaround logic on the underlying log format rules. It also catches the
> > (unlikely) case that the mkfs workaround is applied on the first record
> > incorrectly because h_size is corrupt and there are more records that
> > conflict. Also note that v5 filesystems enforce record CRC checks
> > anyways, so this still might be of limited value...
> 
> based on the words above, my understanding is that the 2nd spilted
> patch is used to do such extra check in xlog_valid_rec_header() to
> 
> check buffer size == rhead->h_size all the time?
> 
> so the relationship would be rhead->h_len <= buffer size = rhead->h_size.
> 
> please correct me if I'm wrong since I'm a bit confusing about some
> English expressions. Yeah, if my understanding is correct, I'd like
> to hear if this 2nd spilted check patch is needed (it won't
> cause any stability impact since rhead->h_size for log records
> other than tail record isn't useful due to the current logic...)
> 

Well I'm not totally convinced the second check is really needed (for
the reasons you mentioned), so this was just an idea of something I
think might be more acceptable than the current approach. The idea was
just to make the validation ensure that h_size is the same across all
log records and it would be completely indepedendent from the mkfs
workaround and the h_len <= bufsize check.

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > > > 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
> > > >
> > >  
> > > 
> > 
>