Re: [PATCH RFC] ubifs: Fix use-after-free in ubifs_tnc_end_commit

[Zhihao Cheng]

在 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);
  }