Fix use-after-free with buffers
PATCH 1/1 Fix use-after-free with buffers
We have a use-after-free issue where log completions access buffers via the buffer log item and the buffer has already been freed. Fix this by taking a reference on the buffer when attaching the buffer log item and release the hold when the buffer log item is detached and we no longer need the buffer. Also create a new function xfs_buf_item_free() to combine some common code and move an ASSERT in xfs_buf_rele() so that we can catch more cases.
===========================================================================fs/xfs/linux-2.6/xfs_buf.c
===========================================================================
fs/xfs/linux-2.6/xfs_buf.c +0 -0
@@ -831,6 +831,8 @@ xfs_buf_rele(
XB_TRACE(bp, "rele", bp->b_relse);
+ ASSERT(atomic_read(&bp->b_hold) > 0);
+
if (unlikely(!hash)) {
ASSERT(!bp->b_relse);
if (atomic_dec_and_test(&bp->b_hold))
@@ -838,7 +840,6 @@ xfs_buf_rele(
return;
}- ASSERT(atomic_read(&bp->b_hold) > 0);
if (atomic_dec_and_lock(&bp->b_hold, &hash->bh_lock)) {
if (bp->b_relse) {
atomic_inc(&bp->b_hold);===========================================================================fs/xfs/xfs_buf_item.c
===========================================================================
fs/xfs/xfs_buf_item.c +0 -0
@@ -732,6 +732,7 @@ xfs_buf_item_init(
bip->bli_item.li_ops = &xfs_buf_item_ops; bip->bli_item.li_mountp = mp; bip->bli_buf = bp; + xfs_buf_hold(bp); bip->bli_format.blf_type = XFS_LI_BUF; bip->bli_format.blf_blkno = (__int64_t)XFS_BUF_ADDR(bp); bip->bli_format.blf_len = (ushort)BTOBB(XFS_BUF_COUNT(bp)); @@ -867,6 +868,23 @@ xfs_buf_item_dirty( return (bip->bli_flags & XFS_BLI_DIRTY); }
+void
+xfs_buf_item_free(
+ xfs_buf_log_item_t *bip)
+{
+#ifdef XFS_TRANS_DEBUG
+ kmem_free(bip->bli_orig);
+ bip->bli_orig = NULL;
+ kmem_free(bip->bli_logged);
+ bip->bli_logged = NULL;
+#endif /* XFS_TRANS_DEBUG */
+
+#ifdef XFS_BLI_TRACE
+ ktrace_free(bip->bli_trace);
+#endif
+ kmem_zone_free(xfs_buf_item_zone, bip);
+}
+
/*
* This is called when the buf log item is no longer needed. It should
* free the buf log item associated with the given buffer and clear
@@ -887,18 +905,8 @@ xfs_buf_item_relse(
(XFS_BUF_IODONE_FUNC(bp) != NULL)) {
XFS_BUF_CLR_IODONE_FUNC(bp);
}-
-#ifdef XFS_TRANS_DEBUG
- kmem_free(bip->bli_orig);
- bip->bli_orig = NULL;
- kmem_free(bip->bli_logged);
- bip->bli_logged = NULL;
-#endif /* XFS_TRANS_DEBUG */
-
-#ifdef XFS_BLI_TRACE
- ktrace_free(bip->bli_trace);
-#endif
- kmem_zone_free(xfs_buf_item_zone, bip);
+ xfs_buf_rele(bp);
+ xfs_buf_item_free(bip);
}
@@ -1120,6 +1128,7 @@ xfs_buf_iodone(
ASSERT(bip->bli_buf == bp);
+ xfs_buf_rele(bp); mp = bip->bli_item.li_mountp; /* @@ -1136,18 +1145,7 @@ xfs_buf_iodone( * xfs_trans_delete_ail() drops the AIL lock. */ xfs_trans_delete_ail(mp, (xfs_log_item_t *)bip);
-
-#ifdef XFS_TRANS_DEBUG
- kmem_free(bip->bli_orig);
- bip->bli_orig = NULL;
- kmem_free(bip->bli_logged);
- bip->bli_logged = NULL;
-#endif /* XFS_TRANS_DEBUG */
-
-#ifdef XFS_BLI_TRACE
- ktrace_free(bip->bli_trace);
-#endif
- kmem_zone_free(xfs_buf_item_zone, bip);
+ xfs_buf_item_free(bip);
}
#if defined(XFS_BLI_TRACE)
Review xfs_buf_rele( · line 841
Quoted 12 lines · Lachlan McIlroy
XB_TRACE(bp, "rele", bp->b_relse);(quoted diff) +2
-0+ ASSERT(atomic_read(&bp->b_hold) > 0); +if (unlikely(!hash)) { ASSERT(!bp->b_relse); if (atomic_dec_and_test(&bp->b_hold))(quoted diff) +0
-1@@ -838,7 +840,6 @@ xfs_buf_rele( return; } - ASSERT(atomic_read(&bp->b_hold) > 0);
Makes sense, but how is this related to the other bits of the patch? All but log and iozero buffers should always be hashed.
Review xfs_buf_item_init( · line 735
Quoted 5 lines
(quoted diff) +1
-0@@ -732,6 +732,7 @@ xfs_buf_item_init( bip->bli_item.li_ops = &xfs_buf_item_ops; bip->bli_item.li_mountp = mp; bip->bli_buf = bp; + xfs_buf_hold(bp);
These refcount changes look good to me.
Quoted 6 lines
(quoted diff) +6
-0+void +xfs_buf_item_free( + xfs_buf_log_item_t *bip) +{ +#ifdef XFS_TRANS_DEBUG + kmem_free(bip->bli_orig);
Review xfs_buf_item_init(
Quoted 10 lines
(quoted diff) +10
-0+ bip->bli_orig = NULL; + kmem_free(bip->bli_logged); + bip->bli_logged = NULL; +#endif /* XFS_TRANS_DEBUG */ + +#ifdef XFS_BLI_TRACE + ktrace_free(bip->bli_trace); +#endif + kmem_zone_free(xfs_buf_item_zone, bip); +}
Faktoring this out makes sense, although you might want to remove the zeroing of bip->bli_orig and bip->bli_logged while you're at it, so that slab poisoning can do it's work.
Quoted 12 lines · Christoph Hellwig
Quoted 11 lines · Lachlan McIlroy
XB_TRACE(bp, "rele", bp->b_relse);(quoted diff) +2
-0+ ASSERT(atomic_read(&bp->b_hold) > 0); +if (unlikely(!hash)) { ASSERT(!bp->b_relse); if (atomic_dec_and_test(&bp->b_hold))(quoted diff) +0
-0@@ -838,7 +840,6 @@ xfs_buf_rele( return; }
Quoted 4 lines
Review xfs_buf_rele( · line 841
Quoted 1 line
(quoted diff) +0
-1- ASSERT(atomic_read(&bp->b_hold) > 0);Makes sense, but how is this related to the other bits of the patch? All but log and iozero buffers should always be hashed.
Okay, not specifically related. I'll split it out of this change.
Quoted 21 lines
Review xfs_buf_item_init( · line 735
Quoted 6 lines
(quoted diff) +1
-0@@ -732,6 +732,7 @@ xfs_buf_item_init( bip->bli_item.li_ops = &xfs_buf_item_ops; bip->bli_item.li_mountp = mp; bip->bli_buf = bp; + xfs_buf_hold(bp);These refcount changes look good to me.
Quoted 12 lines
(quoted diff) +12
-0+void +xfs_buf_item_free( + xfs_buf_log_item_t *bip) +{ +#ifdef XFS_TRANS_DEBUG + kmem_free(bip->bli_orig); + bip->bli_orig = NULL; + kmem_free(bip->bli_logged); + bip->bli_logged = NULL; +#endif /* XFS_TRANS_DEBUG */ + +#ifdef XFS_BLI_TRACE
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ ktrace_free(bip->bli_trace); +#endif + kmem_zone_free(xfs_buf_item_zone, bip); +}Faktoring this out makes sense, although you might want to remove the zeroing of
bip->bli_origandbip->bli_loggedwhile you're at it, so that slab poisoning can do it's work.
Okay, thanks, I'll post a V2.