On Sat, Jul 15, 2023 at 02:36:47PM +0800, Long Li wrote:
> KASAN report a uaf when recover intents fails:
....
>
> If process intents fails, intent items left in AIL will be delete
> from AIL and freed in error handling, even intent items that have been
> recovered and created done items. After this, uaf will be triggered when
> done item commited, because at this point the released intent item will
> be accessed.
>
> xlog_recover_finish xlog_cil_push_work
> ---------------------------- ---------------------------
> xlog_recover_process_intents
> xfs_cui_item_recover//cui_refcount == 1
> xfs_trans_get_cud
> xfs_trans_commit
> <add cud item to cil>
> xfs_cui_item_recover
> <error occurred and return>
> xlog_recover_cancel_intents
> xfs_cui_release //cui_refcount == 0
> xfs_cui_item_free //free cui
> <release other intent items>
> xlog_force_shutdown //shutdown
> <...>
> <push items in cil>
> xlog_cil_committed
> xfs_cud_item_release
> xfs_cui_release // UAF
Huh. The log stores items in the AIL without holding a reference to
them, then on shutdown takes the intent done reference away because
it assumes the intent has not been processed as it is still in the
AIL.
Ok, that's broken.
> Fix it by move log force forward to make sure done items committed before
> cancel intents.
That doesn't fix the fact we have a reference counted object that is
being accessed by code that doesn't actually own a reference to the
object. Intent log items are created with a reference count of 2 -
one for the creator, and one for the intent done object.
Look at xlog_recover_cui_commit_pass2():
/*
* Insert the intent into the AIL directly and drop one reference so
* that finishing or canceling the work will drop the other.
*/
xfs_trans_ail_insert(log->l_ailp, &cuip->cui_item, lsn);
xfs_cui_release(cuip);
return 0;
}
Log recovery explicitly drops the creator reference after it is
inserted into the AIL, but it then processes the log item as if it
also owns the intent-done reference. The moment we call
->iop_recover(), the intent-done reference should be owned by the
log item.
The recovery of the BUI, RUI and EFI all do the same thing. I
suspect that these references should actually be held by log
recovery until it is done processing the item, at which point it
should be removed from the AIL by xlog_recover_process_intents().
The code in ->iop_recovery should assume that it passes the
reference to the done intent, but if that code fails before creating
the done-intent then it needs to release the intent reference
itself.
That way when we go to cancel the intent, the only intents we find
in the AIL are the ones we know have not been processed yet and
hence we can safely drop both the creator and the intent done
reference from xlog_recover_cancel_intents().
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
