Re: [PATCH] xfs: fix invalid pointer dereference in xfs_attr3_node_inactive

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Feb 05, 2020 at 11:58:30AM +0800, Zorro Lang wrote:
> On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote:
> > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote:
> > > This patch fixes below KASAN report. The xfs_attr3_node_inactive()
> > > gets 'child_bp' at there:
> > >   error = xfs_trans_get_buf(*trans, mp->m_ddev_targp,
> > >                             child_blkno,
> > >                             XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0,
> > >                             &child_bp);
> > >   if (error)
> > >           return error;
> > >   error = bp->b_error;
> > > 
> > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by:
> > >   xfs_trans_brelse(*trans, bp);
> > 
> > ....
> > > ---
> > >  fs/xfs/xfs_attr_inactive.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > > index bbfa6ba84dcd..26230d150bf2 100644
> > > --- a/fs/xfs/xfs_attr_inactive.c
> > > +++ b/fs/xfs/xfs_attr_inactive.c
> > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive(
> > >  				&child_bp);
> > >  		if (error)
> > >  			return error;
> > > -		error = bp->b_error;
> > > +		error = child_bp->b_error;
> > >  		if (error) {
> > >  			xfs_trans_brelse(*trans, child_bp);
> > >  			return error;
> > 
> > Isn't this dead code now? i.e. any error that occurs on the buffer
> > during a xfs_trans_get_buf() call is returned directly and so it's
> > caught by the "if (error)" check. Hence this whole child_bp->b_error
> > check can be removed, right?
> 
> Thanks, by looking into the xfs_trans_get_buf() code, I think you're right. Sorry
> I didn't recognise that before.
> 
> But when should we check the bp->b_error? and when's it not necessary?
> In other words, when XFS set the bp->b_error? Looks like it's set in some *verify*
> functions and ioend time?

"Always check b_error after reading."

But please do note that the the buffer read functions will return it for
you now, so you don't have to check it separately in those cases.

(The verifiers and ioend functions are lower level and have to check it
explicitly.)

--D

> Thanks,
> Zorro
> 
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
>