xfsprogs: fix use after free in inode_item_done()

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.

Quoted 9 lines · 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.

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.

Quoted 16 lines · Dave Chinner

Quoted 9 lines · 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.

which leaves the rest of the ili_done: code looking a little strange.

can you convert that now to be:

Quoted 8 lines

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

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 <[email protected]>

---

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

On 3/3/14, 4:36 PM, Dave Chinner wrote:

Quoted 5 lines

which leaves the rest of the ili_done: code looking a little strange.

can you convert that now to be:

Quoted 8 lines

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

Commit "3a19fb7 libxfs: stop caching inode structures" introduced a use after free.

Quoted 2 lines · Eric Sandeen

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.

Quoted 6 lines

libxfs/trans.c +0 -2

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

libxfs/trans.c ili_done: · line 741

Quoted 5 lines

libxfs/trans.c +0 -4

-		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.

Quoted 6 lines · Eric Sandeen

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;

Quoted 5 lines

        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.

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