Re: [PATCH] xfsprogs: fix use after free in inode_item_done()

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 03, 2014 at 04:48:29PM -0600, Eric Sandeen wrote:
> On 3/3/14, 4:36 PM, Dave Chinner wrote:
> > On Mon, Mar 03, 2014 at 02:41:54PM -0600, Eric Sandeen wrote:
> >> Commit "3a19fb7 libxfs: stop caching inode structures"
> >> introduced a use after free.
> >>
> >> libxfs_iput() already does the check for ip->i_itemp, and a
> >> kmem_zone_free() if it's present, and then frees the ip pointer.
> >> Re-checking ip->i_itemp after the libxfs_iput call will access
> >> the freed ip pointer, as will setting ip_>i_itemp to NULL.
> >>
> >> Simply remove the offending code to fix this up.
> > 
> > which leaves the rest of the ili_done: code looking a little
> > strange.
> > 
> > can you convert that now to be:
> > 
> > ili_done:
> > 	if (iip->ili_lock_flags) {
> > 		iip->ili_lock_flags = 0;
> > 		return;
> > 	}
> > 	/* free the inode */
> > 	libxfs_iput(ip, 0);
> > }
> 
> yeah, I actually had that first.  Not sure why I didn't go with it ;)
> 
> (Still looks strange to my untrained eye; "if lock flags are set, unset them and don't free the inode, otherwise free it")

If the lock falgs are set, it means the caller expects the
transaction commit to return the inode to it in a locked state.
Exactly the same semantics as the kernel code - the lock flags track
how the transaction releases the inode...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs