Fix use-after-free with buffers
PATCH v2 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/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,21 @@ 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);
+ kmem_free(bip->bli_logged);
+#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 +903,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 +1126,7 @@ xfs_buf_iodone(
ASSERT(bip->bli_buf == bp);
+ xfs_buf_rele(bp); mp = bip->bli_item.li_mountp; /* @@ -1136,18 +1143,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)
Quoted 7 lines · Lachlan McIlroy
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 inxfs_buf_rele()so that we can catch more cases.
Looks good, but please mark xfs_buf_item_free static.
Quoted 74 lines
fs/xfs/xfs_buf_item.c +19
-18@@ -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,21 @@ 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); +kmem_free (bip->bli_logged); +#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 +903,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 +1126,7 @@ xfs_buf_iodone(ASSERT (bip->bli_buf == bp); +xfs_buf_rele (bp); mp = bip->bli_item.li_mountp;/* @@ -1136,18 +1143,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 ;
fs/xfs/xfs_buf_item.c xfs_buf_iodone( · line 1149
Quoted 10 lines
fs/xfs/xfs_buf_item.c +1
-6-#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 )
---end quoted text---