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

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

On Tue, Nov 11, 2025 at 10:44:26PM -0500, Chris Mason wrote:

> We're wandering into fuzzing territory here, and I honestly have no idea
> if this is a valid use of any of this code, but AI managed to make a
> repro that crashes only after your patch.  So, I'll let you decide.
> 
> The new review:
> 
> Can this dereference ZERO_SIZE_PTR when eps_count is 0?
> 
> When ffs->eps_count is 0, ffs_epfiles_create() calls kcalloc(0, ...) which
> returns ZERO_SIZE_PTR (0x10). The loop never executes so epfiles[0].ffs is
> never initialized. Later, cleanup paths (ffs_data_closed and ffs_data_clear)
> check if (epfiles) which is true for ZERO_SIZE_PTR, and call
> ffs_epfiles_destroy(epfiles, 0).
> 
> In the old code, the for loop condition prevented any dereferences when
> count=0. In the new code, "root = epfile->ffs->sb->s_root" dereferences
> epfile before checking count, which would fault on ZERO_SIZE_PTR.

Lovely.  OK, this is a bug.  It is trivial to work around (all callers
have ffs avaible, so just passing it as an explicit argument solves
the problem), but there is a real UAF in functionfs since all the way
back to original merge.  Take a look at

static int
ffs_epfile_open(struct inode *inode, struct file *file)
{
	struct ffs_epfile *epfile = inode->i_private;

	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
		return -ENODEV;

	file->private_data = epfile;
	ffs_data_opened(epfile->ffs);

	return stream_open(inode, file);
}

and think what happens if that (->open() of dynamic files in there)
races with file removal.  Specifically, if we get called with ffs->opened
equal to 1 due to opened ep0 and get preempted away just before the
call ffs_data_opened().  Another thread closes ep0, hitting
ffs_data_closed(), dropping ffs->opened to 0 and getting
			ffs->state = FFS_CLOSING;
			ffs_data_reset(ffs);
which calls ffs_data_clear(), where we hit
		ffs_epfiles_destroy(epfiles, ffs->eps_count);
All files except ep0 are removed and epfiles gets freed, leaving the
first thread (in ffs_epfile_open()) with file->private_data pointing
into a freed array.

open() succeeds, with any subsequent IO on the resulting file leading
to calls of
static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
{
	struct ffs_epfile *epfile = file->private_data;

and a bunch of accesses to *epfile later in that function, all of them
UAF.

As far as I can tell, the damn thing intends to prevent removals between
ffs_data_opened() and ffs_data_closed(), so other methods would be safe
if ->open() had been done right.  I'm not happy with the way that FSM
is done (the real state is a mix of ffs->state, ffs->opened and ffs->mutex,
and rules bloody awful; I'm still not entirely convinced that ffs itself
can't be freed with ffs->reset_work scheduled for execution), but that's
a separate story.  

Another variant of that scenario is with ffs->no_disconnect set;
in a sense, it's even nastier.  In that case ffs_data_closed() won't
remove anything - it will set ffs->state to FFS_DEACTIVATED, leaving
the removals for ffs_data_open().  If we have *two* threads in open(),
the first one to call ffs_data_open() will do removal; on another CPU
the second will just get past its increment of ->opened (from 1 to 2)
and move on, without waiting for anything.

IMO we should just take ffs->mutex in there, getting to ffs via
inode->i_sb->s_fs_info.  And yes, compare ffs->state with FFS_ACTIVE -
under ->mutex, without WARN_ON() and after having bumped ->opened
so that racing ffs_data_closed() would do nothing.  Not FFS_ACTIVE -
call ffs_data_closed() ourselves on failure exit.

As in

static int
ffs_epfile_open(struct inode *inode, struct file *file)
{
	strict ffs_data *ffs = inode->i_sb->s_fs_info;
	int ret;

        /* Acquire mutex */
	ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK);
	if (ret < 0)
		return ret;

	ffs_data_opened(ffs);
	/*
	 * not FFS_ACTIVE - there might be a pending removal;
	 * FFS_ACITVE alone is not enough, though - we might have
	 * been through FFS_CLOSING and back to FFS_ACTIVE,
	 * with our file already removed.
	 */
	if (unlikely(ffs->state != FFS_ACTIVE ||
		     !simple_positive(file->f_path.dentry))) {
		ffs_data_closed(ffs);
		mutex_unlock(&ffs->mutex);
		return -ENODEV;
	}
	mutex_unlock(&ffs->mutex);

	file->private_data = inode->i_private;
	return stream_open(inode, file);
}

and

static int ffs_ep0_open(struct inode *inode, struct file *file)
{
        struct ffs_data *ffs = inode->i_private;
	int ret;

        /* Acquire mutex */
	ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK);
	if (ret < 0)
		return ret;

	ffs_data_opened(ffs);
	if (ffs->state == FFS_CLOSING) {
		ffs_data_closed(ffs);
		mutex_unlock(&ffs->mutex);
		return -EBUSY;
	}
	mutex_unlock(&ffs->mutex);

	file->private_data = ffs;
	return stream_open(inode, file);
}

Said that, I'm _NOT_ familiar with that code; this is just from a couple
of days digging through the driver, so I would like to hear comments from
the maintainer...  Greg?