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

[Al Viro <viro@zeniv.linux.org.uk>]

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.