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

[Zorro Lang <zlang@redhat.com>]

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?

Thanks,
Zorro

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