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?
