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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Nov 13, 2025 at 09:26:36AM +0000, Al Viro wrote:
> 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?
> 

Sorry for the delay.  Yes, we should be grabing the mutex in there, good
catch.  There's been more issues pointed out with the gadget code in the
past year or so as more people are starting to actually use it and
stress it more.  So if you have a patch for this, I'll gladly take it :)

thanks,

greg k-h