Re: [PATCH] usb: gadget: f_fs: fix use-after-free in ffs_closed() on umount

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: ffs: TOCTOU use-after-free in ffs_data_put()
--

commit 215b73da4a8e129ed558d6bf4fd1b28619b9f490
Author: Mohammad Mosafer <mohsafer@gmail.com>

    usb: gadget: f_fs: fix use-after-free in ffs_closed() on umount
    
    This commit fixes a use-after-free in ffs_closed() during umount by holding
    ffs_dev_lock across the release and free operations in ffs_free_inst().
    It splits ffs_release_dev() into a lockless _ffs_release_dev() and a locked
    wrapper to ensure the device cannot be found by a concurrent mount(2) while
    it is being torn down.
    
    Reported-by: syzbot+6227549bd2c8a1ec8ba0@syzkaller.appspotmail.com
    Closes: https://syzkaller.appspot.com/bug?extid=6227549bd2c8a1ec8ba0
    Fixes: 5920cda627688c ("usb: gadget: FunctionFS: convert to new function interface with backward compatibility")

> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index c64a268e98a43..5e7179b4250ef 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c

[ ... ]

> @@ -4147,8 +4148,17 @@ static void ffs_free_inst(struct usb_function_instance *f)
>  	struct f_fs_opts *opts;
>  
>  	opts = to_f_fs_opts(f);
> -	ffs_release_dev(opts->dev);
> +
> +	/*
> +	 * Release and free the dev under a single ffs_dev_lock critical
> +	 * section. Between ffs_release_dev() and _ffs_free_dev() the dev
> +	 * would still be on the ffs_devices list while already unmounted,
> +	 * so a concurrent ffs_acquire_dev() could link a fresh ffs_data to
> +	 * the doomed dev, leaving it with a dangling ->private_data that is
> +	 * dereferenced in ffs_closed() when that mount is torn down.
> +	 */
>  	ffs_dev_lock();
> +	_ffs_release_dev(opts->dev);
>  	_ffs_free_dev(opts->dev);
>  	ffs_dev_unlock();
>  	kfree(opts);

[Severity: High]
Is it possible for this change to trigger a time-of-check to time-of-use
use-after-free of the ffs_dev object?

Looking at ffs_data_put() in drivers/usb/gadget/function/f_fs.c, it evaluates
the device pointer locklessly before calling ffs_release_dev():

ffs_data_put()
    ...
    ffs_release_dev(ffs->private_data);

If a concurrent configfs_rmdir() invokes ffs_free_inst(), could the following
sequence occur?

Thread 1
ffs_data_put()
    evaluates ffs->private_data (obtaining the ffs_dev pointer)
    calls ffs_release_dev(), blocking on ffs_dev_lock()

Thread 2
ffs_free_inst()
    ffs_dev_lock();
    _ffs_release_dev(opts->dev);
    _ffs_free_dev(opts->dev); // device is freed here
    ffs_dev_unlock();

Thread 1
ffs_release_dev()
    acquires ffs_dev_lock()
    _ffs_release_dev(ffs_dev) // ffs_dev is now stale

Inside _ffs_release_dev(), the code checks:

    if (ffs_dev && ffs_dev->mounted)

This would dereference the stale ffs_dev pointer.

Can this sequence occur when userspace unmounts and manipulates configfs
concurrently?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006210523.150724-1-mohsafer@gmail.com?part=1