Re: [PATCH] xfs: fix use-after-free in xattr node block inactivation

[Christoph Hellwig <hch@lst.de>]

On Fri, Jul 08, 2022 at 02:26:53PM +1000, Dave Chinner wrote:
> > 			child_blkno,
> > 			XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0,
> > 			&child_bp);
> > 	if (error)
> > 		return error;
> > 	error = bp->b_error;
> > 
> > That doesn't look right -- I think this should be dereferencing
> > child_bp, not bp.
> 
> It shouldn't even be there. If xfs_trans_get_buf() returns a buffer,
> it should not have a pending error on it at all. i.e. it's supposed
> to return either an error or a buffer handle that is ready for use.

Agreed. Consumers of the buffer cache API should never look at b_error
because they will not see buffers with b_error set at all.

> Whoever wrote this didn't, for some reason, use the da btree path
> tracking (i.e. a struct xfs_da_state) to keep track of all the
> parent buffers of the current child being invalidated. That would
> make this code a whole lot simpler and neater....

Yeah.  The brelese seems to go back to:

commit 677821a1ab2301629aa0370835babb33bc6c919e
Author: Doug Doucette <doucette@engr.sgi.com>
Date:   Fri Dec 6 22:05:46 1996 +0000

    Fold in ficus changes not yet merged in:
    revision 1.32
    date: 1996/11/21 23:31:08;  author: doucette;  state: Exp;  lines: +69 -205
    Rewrite inactive attribute code to avoid freeing any of the data blocks
    until the very end.  We still walk the on-disk structure, but just
    call xfs_trans_binval on the buffers we get.  Then we call the truncate
    code to get rid of the data blocks.  This means we don't need a block
    reservation.

and the loop іtself is even older.  But the da_state had been around
since 1996, so that isn't really an excuse.

> > +		error = child_bp->b_error;
> >  		if (error) {
> >  			xfs_trans_brelse(*trans, child_bp);
> >  			return error;
> 
> I'd just remove the child_bp error checking altogether - if there
> was an IOi or corruption error on it, that shouldn't keep us from
> invalidating it to free the underlying space. We're trashing the
> contents, so who cares if the contents is already trashed?

Yeah.  I also don't see how a b_error could even magically appear
here without xfs_trans_get_buf returning an error first.

> Also, you probably need to set bp = NULL after the
> xfs_trans_brelse() call at the bottom of the loop....

Yes.