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

[Chris Mason <clm@meta.com>]

On 11/13/25 4:20 PM, Greg Kroah-Hartman wrote:
> 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.

I was curious what else would get flagged if I ran the whole f_fs.c through
the review prompt.  It found a variant of Al's bug above, along with additional
concerns around unprotected ffs->gadget?  BUGS #1 and #2 below look
the most important, did AI miss some locking there?

-chris

================================================================================
BUG #1: NULL pointer dereference in ffs_dmabuf_find_attachment()
================================================================================

In ffs_dmabuf_find_attachment(), the gadget pointer is dereferenced without
a NULL check:

static struct dma_buf_attachment *
ffs_dmabuf_find_attachment(struct ffs_epfile *epfile, struct dma_buf *dmabuf)
{
        struct device *dev = epfile->ffs->gadget->dev.parent;
                                           ^^^^^^^

Can epfile->ffs->gadget be NULL here? This function is called from dmabuf
ioctl handlers which can execute at any time the file is open. If the USB
function is unbound or the gadget is being torn down, gadget will be NULL.

================================================================================
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().

================================================================================
BUG #3: race with gadget pointer in ffs_dmabuf_attach()
================================================================================

In ffs_dmabuf_attach(), the gadget pointer is checked then used without
synchronization:

static int ffs_dmabuf_attach(struct file *file, int fd)
{
        ...
        struct usb_gadget *gadget = epfile->ffs->gadget;
        ...
        if (!gadget || !gadget->sg_supported)
                return -EPERM;

        dmabuf = dma_buf_get(fd);
        if (IS_ERR(dmabuf))
                return PTR_ERR(dmabuf);

        attach = dma_buf_attach(dmabuf, gadget->dev.parent);
                                        ^^^^^^^

Can the gadget be freed between the NULL check and use? The gadget pointer
is cached early in the function, checked for NULL, but then used later
without any locks. If the function is unbound between the check and use,
gadget->dev.parent dereferences freed memory.

A mutex is held when ffs->gadget is set to NULL, but ffs_dma_buff_attach
is Called from ff_epfile_ioctl(), with no locks are held.

================================================================================
BUG #4: dma_fence_put on uninitialized fence in ffs_dmabuf_transfer()
================================================================================

In ffs_dmabuf_transfer(), error paths call dma_fence_put() before the fence
is initialized:

static int ffs_dmabuf_transfer(...)
{
        ...
        fence = kmalloc(sizeof(*fence), GFP_KERNEL);
        if (!fence) {
                ret = -ENOMEM;
                goto err_resv_unlock;
        }

        fence->priv = priv;

        spin_lock_irq(&epfile->ffs->eps_lock);

        if (epfile->ep != ep) {
                ret = -ESHUTDOWN;
                goto err_fence_put;
        }

        usb_req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
        if (!usb_req) {
                ret = -ENOMEM;
                goto err_fence_put;
        }

        seqno = atomic_add_return(1, &epfile->seqno);

        dma_fence_init(&fence->base, &ffs_dmabuf_fence_ops,
                       &priv->lock, priv->context, seqno);
        ...

err_fence_put:
        spin_unlock_irq(&epfile->ffs->eps_lock);
        dma_fence_put(&fence->base);
        ...
}

Can dma_fence_put() be called on an uninitialized fence? The error paths at
epfile->ep != ep and usb_ep_alloc_request failure jump to err_fence_put, but
dma_fence_init() isn't called until after those checks. Calling
dma_fence_put() on an uninitialized fence violates the DMA fence API and
likely crashes on uninitialized refcount.

================================================================================
BUG #5: NULL pointer dereference in ffs_epfile_ioctl()
================================================================================

In ffs_epfile_ioctl() handling FUNCTIONFS_ENDPOINT_DESC, the gadget pointer
is dereferenced without a NULL check:

static long ffs_epfile_ioctl(...)
{
        ...
        case FUNCTIONFS_ENDPOINT_DESC:
        {
                int desc_idx;
                struct usb_endpoint_descriptor desc1, *desc;

                switch (epfile->ffs->gadget->speed) {
                                   ^^^^^^^

Can epfile->ffs->gadget be NULL here? The gadget can be
NULL if the function is unbound. The function holds eps_lock but this
doesn't protect against gadget being NULL.

================================================================================
BUG #6: NULL pointer dereference accessing descriptor array
================================================================================

In the same FUNCTIONFS_ENDPOINT_DESC handler, the descriptor pointer from
the array is used without NULL check:

                switch (epfile->ffs->gadget->speed) {
                case USB_SPEED_SUPER:
                case USB_SPEED_SUPER_PLUS:
                        desc_idx = 2;
                        break;
                case USB_SPEED_HIGH:
                        desc_idx = 1;
                        break;
                default:
                        desc_idx = 0;
                }

                desc = epfile->ep->descs[desc_idx];
                memcpy(&desc1, desc, desc->bLength);
                                     ^^^^

Can desc be NULL here? The descs array elements may not all be populated if
userspace only provided descriptors for certain speeds. Accessing
desc->bLength without a NULL check can crash.

================================================================================
BUG #7: Out-of-bounds array access in ffs_func_get_alt()
================================================================================

In ffs_func_get_alt(), the interface parameter is used to index cur_alt[]
instead of the validated intf value:

static int ffs_func_get_alt(struct usb_function *f,
                            unsigned int interface)
{
        struct ffs_function *func = ffs_func_from_usb(f);
        int intf = ffs_func_revmap_intf(func, interface);

        return (intf < 0) ? intf : func->cur_alt[interface];
                                                    ^^^^^^^^^
}

Can func->cur_alt[interface] overflow the array? The function calls
ffs_func_revmap_intf() to validate and map the interface number, returning
the validated index in intf. However, it then uses the unvalidated
interface parameter to index cur_alt[] instead of intf.

If interface >= MAX_CONFIG_INTERFACES, this reads beyond the array bounds.

ffs_func_set_alt() follows the same incorrect pattern:
func->cur_alt[interface] = alt;

Data flow analysis:

The interface parameter originates from the USB HOST (the PC or device that
the USB gadget is plugged into), NOT from the userspace application. Here's
the call chain:

1. USB HOST sends USB_REQ_GET_INTERFACE control request over the wire
2. composite_setup() in drivers/usb/gadget/composite.c handles it
3. Extracts w_index from ctrl->wIndex (16-bit value from USB packet)
4. Validates LOW 8 bits: checks intf >= MAX_CONFIG_INTERFACES
5. Gets function: f = cdev->config->interface[intf]
6. Calls: value = f->get_alt(f, w_index)

The composite layer validates the low 8 bits (intf), but passes the FULL
16-bit w_index as the interface parameter. The FunctionFS code:

1. Calls ffs_func_revmap_intf(func, interface) which validates and returns
   a local index
2. **But then uses the original interface parameter to index cur_alt[]
   instead of the validated intf**

The interface number comes from the USB HOST over the wire,
not from userspace application. It's validated partially by composite, but
FunctionFS uses the wrong variable for array indexing.