Re: [PATCH RFC] ubifs: Fix use-after-free in ubifs_tnc_end_commit
From: Zhihao Cheng
Date: Tue Oct 29 2024 - 07:19:28 EST
在 2024/10/29 17:56, Ryder Wang 写道:
Thanks for reporting that :-). I noticed it a period time ago too, and I
found 'c->znext', 'c->cnext' and 'znode->cnext' won't be accessed (in
write mode) by other tasks, because there is only one function
do_commit() modifying them and do_commit() can be executed by at most
one task in any time.
It looks the race condition can really happen in this case from the issue reporter.
1. do_commit (ubifs_bg_thread): it can finally touch unprotected znode while calling the function write_index().
2. ubifs_evict_inode (other kernel thread than ubifs_bg_thread): it can finally touch the znode in the function tnc_delete(). Even there is mutex protection for tnc_delete(), but it has no meaning because of do_commit (at the point 1) doesn't have such mutex protection while calling write_index().
.
Emm.., this has been discussed in previous sessions.
All dirty znodes are gathered by function get_znodes_to_commit() which
is protected by c->tnc_mutex, and the 'cparent' member in all dirty
znodes is assigned with non-NULL. Then I think the znode memory freeing
path 'tnc_delete->kfree(znode)' cannot happen, because:
1) If a znode is dirtied, all its' ancestor znodes(a path from znode to
root znode) must be dirtied, which is guaranteed by UBIFS. See
dirty_cow_bottom_up/lookup_level0_dirty.
2) A dirty znode waiting for commit cannot be freed before write_index()
finished, which is guaranteed by tnc_delete:
if (znode->cnext) {
__set_bit(OBSOLETE_ZNODE, &znode->flags);
...
} else {
kfree(znode);
}