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

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

[Eric Sandeen]

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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/libxfs/trans.c b/libxfs/trans.c
index 6c9d202..2e6720e 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -694,7 +694,6 @@ inode_item_done(
 	xfs_mount_t		*mp;
 	xfs_buf_t		*bp;
 	int			error;
-	extern kmem_zone_t	*xfs_ili_zone;
 
 	ip = iip->ili_inode;
 	mp = iip->ili_item.li_mountp;
@@ -739,12 +738,6 @@ ili_done:
 	} else {
 		libxfs_iput(ip, 0);
 	}
-
-	if (ip->i_itemp)
-		kmem_zone_free(xfs_ili_zone, ip->i_itemp);
-	else
-		ASSERT(0);
-	ip->i_itemp = NULL;
 }
 
 static void


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

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

[Dave Chinner]

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);
}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Eric Sandeen]

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")

Anyway, I'll resend.  No need to educate me on these details, for now.  ;)

-Eric

> Cheers,
> 
> Dave.
> 

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

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

[Dave Chinner]

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

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

[Eric Sandeen]

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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Make it prettier based on Dave's comments.

diff --git a/libxfs/trans.c b/libxfs/trans.c
index 6c9d202..c443863 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -694,7 +694,6 @@ inode_item_done(
 	xfs_mount_t		*mp;
 	xfs_buf_t		*bp;
 	int			error;
-	extern kmem_zone_t	*xfs_ili_zone;
 
 	ip = iip->ili_inode;
 	mp = iip->ili_item.li_mountp;
@@ -736,15 +735,9 @@ ili_done:
 	if (iip->ili_lock_flags) {
 		iip->ili_lock_flags = 0;
 		return;
-	} else {
-		libxfs_iput(ip, 0);
 	}
-
-	if (ip->i_itemp)
-		kmem_zone_free(xfs_ili_zone, ip->i_itemp);
-	else
-		ASSERT(0);
-	ip->i_itemp = NULL;
+	/* free the inode */
+	libxfs_iput(ip, 0);
 }
 
 static void

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

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

[Dave Chinner]

On Mon, Mar 03, 2014 at 04:58:44PM -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.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: Make it prettier based on Dave's comments.

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

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

[Roger Willcocks]

On Mon, 2014-03-03 at 16:48 -0600, Eric Sandeen wrote:
> On 3/3/14, 4:36 PM, Dave Chinner wrote:
...
> > 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")
> 

I'd be tempted to write:

ili_done:
	if (iip->ili_lock_flags == 0) /* don't return locked inode */
		libxfs_iput(ip, 0);

	iip->ili_lock_flags = 0;

-- 
Roger Willcocks <roger@filmlight.ltd.uk>

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

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

[Christoph Hellwig]

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.

I see the use after free, but I don't see how I would have introduced it
in that commit.  Before that libxfs_iput already was freeing the inode
through the cache code, now we do it directly.

> @@ -739,12 +738,6 @@ ili_done:
>  	} else {
>  		libxfs_iput(ip, 0);
>  	}
> -
> -	if (ip->i_itemp)
> -		kmem_zone_free(xfs_ili_zone, ip->i_itemp);
> -	else
> -		ASSERT(0);
> -	ip->i_itemp = NULL;
>  }

Seems like inode_item_done should call into inode_item_unlock, which
factors the exact sequence we want into a nice helper.

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

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

[Eric Sandeen]

On 3/5/14, 11:02 AM, Christoph Hellwig 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.
> 
> I see the use after free, but I don't see how I would have introduced it
> in that commit.  Before that libxfs_iput already was freeing the inode
> through the cache code, now we do it directly.

Ok, sorry if I wrongly implicated that commit.

>> @@ -739,12 +738,6 @@ ili_done:
>>  	} else {
>>  		libxfs_iput(ip, 0);
>>  	}
>> -
>> -	if (ip->i_itemp)
>> -		kmem_zone_free(xfs_ili_zone, ip->i_itemp);
>> -	else
>> -		ASSERT(0);
>> -	ip->i_itemp = NULL;
>>  }
> 
> Seems like inode_item_done should call into inode_item_unlock, which
> factors the exact sequence we want into a nice helper.

Yeah, that does seem better!   Thanks for spotting that.

The difference when calling inode_item_unlock is a bit more zeroing-out:

        ip->i_transp = NULL;

        iip->ili_flags = 0;

I'm not sure of the implications of that offhand, TBH.

Dave, hold off on my commit I guess ;)

-Eric

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

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

[Eric Sandeen]

On 3/5/14, 4:36 PM, Christoph Hellwig wrote:
> On Wed, Mar 05, 2014 at 11:19:19AM -0600, Eric Sandeen wrote:
>> Yeah, that does seem better!   Thanks for spotting that.
>>
>> The difference when calling inode_item_unlock is a bit more zeroing-out:
>>
>>         ip->i_transp = NULL;
>>
>>         iip->ili_flags = 0;
>>
>> I'm not sure of the implications of that offhand, TBH.
>>
>> Dave, hold off on my commit I guess ;)
> 
> i_itransp nulling is obviously harmless as we are freeing the inode
> right after.

Not in all cases, right?

static void
inode_item_unlock(
        xfs_inode_log_item_t    *iip)
{
        xfs_inode_t             *ip = iip->ili_inode;

        /* Clear the transaction pointer in the inode. */
        ip->i_transp = NULL;

        iip->ili_flags = 0;
        if (!iip->ili_lock_flags)
                libxfs_iput(ip, 0);
        else
                iip->ili_lock_flags = 0;  // <-- not here.
}



> ili_flags is always 0 in libxfs and we might as well just remove it.
> 

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