Gently ping ...
在 2023/4/20 11:35, yangerkun 写道:
From: yangerkun <yangerkun@xxxxxxxxxx>
commit 84d8949e7707 ("xfs: hold buffer across unpin and potential
shutdown processing") describle a use-after-free bug show as follow.
Call xfs_buf_hold before dec b_pin_count can forbid the problem.
+-----------------------------+--------------------------------+
xlog_ioend_work | xfsaild
... | xfs_buf_delwri_submit_buffers
xfs_buf_item_unpin |
dec &bip->bli_refcount |
dec &bp->b_pin_count |
| // check unpin and go on
| __xfs_buf_submit
| xfs_buf_ioend_fail // shutdown
| xfs_buf_ioend
| xfs_buf_relse
| xfs_buf_free(bp)
xfs_buf_lock(bp) // UAF |
However with the patch, we still get a UAF with shutdown:
+-----------------------------+--------------------------------+
xlog_ioend_work | xlog_cil_push_work // now shutdown
... | xlog_cil_committed
xfs_buf_item_unpin | ...
// bli_refcount = 2 |
dec bli_refcount // 1 | xfs_buf_item_unpin
| dec bli_refcount // 0,will free
| xfs_buf_ioend_fail // free bp
dec b_pin_count // UAF |
xlog_cil_push_work will call xlog_cil_committed once we meet some error
like shutdown, and then call xfs_buf_item_unpin with 'remove' equals 1.
xlog_ioend_work can happened same time which trigger xfs_buf_item_unpin
too, and then bli_refcount will down to zero which trigger
xfs_buf_ioend_fail that free the xfs_buf, so the UAF can trigger.
Fix it by call xfs_buf_hold before dec bli_refcount, and release the
hold once we actually do not need it.
Signed-off-by: yangerkun <yangerkun@xxxxxxxxxx>
---
fs/xfs/xfs_buf_item.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index df7322ed73fa..3880eb2495b8 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -502,12 +502,15 @@ xfs_buf_item_unpin(
* completion) at any point before we return. This can be removed once
* the AIL properly holds a reference on the bli.
*/
+ xfs_buf_hold(bp);
freed = atomic_dec_and_test(&bip->bli_refcount);
- if (freed && !stale && remove)
- xfs_buf_hold(bp);
+
if (atomic_dec_and_test(&bp->b_pin_count))
wake_up_all(&bp->b_waiters);
+ if (!freed || stale || !remove)
+ xfs_buf_rele(bp);
+
/* nothing to do but drop the pin count if the bli is active */
if (!freed)
return;
