Re: [functionfs] mainline UAF (was Re: [PATCH v3 36/50] functionfs: switch to simple_remove_by_name())

Al Viro
2025-11-14T07:58:58Z

On Thu, Nov 13, 2025 at 09:16:52PM -0500, Chris Mason wrote:

> ================================================================================
> BUG #2: Race condition in ffs_data_closed()
> ================================================================================
>
> In ffs_data_closed(), there's an unsynchronized state modification:
>
> static void ffs_data_closed(struct ffs_data *ffs)
> {
> ...
> if (atomic_dec_and_test(&ffs->opened)) {
> if (ffs->no_disconnect) {
> ffs->state = FFS_DEACTIVATED;
> ...
> } else {
> ffs->state = FFS_CLOSING;
> ffs_data_reset(ffs);
> }
> }
> if (atomic_read(&ffs->opened) < 0) {
> ffs->state = FFS_CLOSING;
> ffs_data_reset(ffs);
> }
> ...
> }
>
> Can this race with concurrent state changes? The atomic_read() check is not
> synchronized with the subsequent state assignment. Between the read and the
> assignment, another thread could modify the state, potentially causing state
> machine corruption or double cleanup via ffs_data_reset().

The atomic_read() check is utter bollocks. ->opened starts with 0, is bumped
on opens on that fs and decremented when files are closed. How could it end
up negative? Well might you ask - there's a call of ffs_data_closed() in
ffs_kill_sb(). _There_ we are guaranteed that there's no opened files (we'd
better, or there would be much worse problems), so ffs->opened is known to
be 0 and ffs_data_closed() decrements it to -1.

In other words, that couple of lines is executed on call from ffs_kill_sb()
and only on that call. Which is to say, that thing is just an obfuscated
if (s->s_fs_info) {
struct ffs_data *ffs = s->s_fs_info;
ffs->state = FFS_CLOSING;
ffs_data_reset(ffs);
ffs_data_put(ffs);
}

Not a race, just a highly unidiomatic code. While we are at it, after taking
that mess out of ffs_data_closed() we no longer need to bump/drop ffs->ref
in ffs_data_opened()/ffs_data_closed() - superblock owns a reference all along
and it's *not* going away under an opened file.