Archive-only list for patches
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 6.18-6.17] functionfs: fix the open/removal races
@ 2025-12-10  3:48 Sasha Levin
  2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] scsi: qla2xxx: Use reinit_completion on mbx_intr_comp Sasha Levin
                   ` (18 more replies)
  0 siblings, 19 replies; 20+ messages in thread
From: Sasha Levin @ 2025-12-10  3:48 UTC (permalink / raw)
  To: patches, stable
  Cc: Al Viro, Greg Kroah-Hartman, Sasha Levin, brauner, ingo.rohloff,
	nichen, mjguzik, guhuinan, liangjie, akash.m5

From: Al Viro <viro@zeniv.linux.org.uk>

[ Upstream commit e5bf5ee266633cb18fff6f98f0b7d59a62819eee ]

ffs_epfile_open() can race with removal, ending up with file->private_data
pointing to freed object.

There is a total count of opened files on functionfs (both ep0 and
dynamic ones) and when it hits zero, dynamic files get removed.
Unfortunately, that removal can happen while another thread is
in ffs_epfile_open(), but has not incremented the count yet.
In that case open will succeed, leaving us with UAF on any subsequent
read() or write().

The root cause is that ffs->opened is misused; atomic_dec_and_test() vs.
atomic_add_return() is not a good idea, when object remains visible all
along.

To untangle that
	* serialize openers on ffs->mutex (both for ep0 and for dynamic files)
	* have dynamic ones use atomic_inc_not_zero() and fail if we had
zero ->opened; in that case the file we are opening is doomed.
	* have the inodes of dynamic files marked on removal (from the
callback of simple_recursive_removal()) - clear ->i_private there.
	* have open of dynamic ones verify they hadn't been already removed,
along with checking that state is FFS_ACTIVE.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Summary

### What the Commit Fixes

This commit fixes a **Use-After-Free (UAF)** vulnerability in USB gadget
functionfs. The race condition occurs between `ffs_epfile_open()` and
file removal:

1. Thread A is in `ffs_epfile_open()` but hasn't incremented
   `ffs->opened` yet
2. Thread B closes the last file handle, triggering removal (since
   `opened` is 0)
3. Thread A's open succeeds with `file->private_data` pointing to freed
   memory
4. Subsequent `read()`/`write()` operations cause UAF

### Fix Mechanism

The fix implements proper synchronization:
- Serializes openers using `ffs->mutex`
- Uses `atomic_inc_not_zero()` to fail if counter already zero
- Uses `smp_load_acquire()`/`smp_store_release()` for memory ordering
- Clears `i_private` during removal via `simple_recursive_removal()`
  callback
- Uses `file->private_data` instead of `inode->i_private` in release
  path

### Stable Kernel Criteria Assessment

| Criterion | Assessment |
|-----------|------------|
| Obviously correct | ✅ Uses standard kernel primitives, proper locking
patterns |
| Fixes real bug | ✅ UAF vulnerability, security-relevant |
| Important issue | ✅ Security bug, potential for crashes/exploitation |
| Small and contained | ✅ Single file, +43/-10 lines, localized changes
|
| No new features | ✅ Pure bug fix, no new APIs |

### Risk vs Benefit

**Benefits:**
- Fixes serious UAF vulnerability
- USB gadget functionfs used in Android, embedded systems
- Reviewed by Greg Kroah-Hartman (USB maintainer, stable maintainer)
- Written by Al Viro (highly respected kernel developer)

**Risks:**
- Moderate complexity (changes locking behavior)
- Recent commit (November 2025), limited mainline soak time
- No explicit `Cc: stable@vger.kernel.org` tag

### Dependencies

- `ffs_mutex_lock()` - exists in functionfs since early versions
- `simple_recursive_removal()` with callback - available since ~5.x
  kernels
- Standard kernel APIs (`atomic_inc_not_zero`, memory barriers) -
  universally available

### Concerns

1. **No Fixes: tag** - Makes it harder to determine which stable trees
   need this fix
2. **No Cc: stable tag** - May indicate maintainers wanted soak time, or
   an oversight given Greg KH reviewed it
3. **Backport effort** - May need adjustment for older stable trees
   depending on functionfs evolution

### Conclusion

This is a legitimate UAF security fix that affects real-world users
(Android, embedded USB gadgets). Despite moderate complexity, the fix:
- Addresses a serious vulnerability class (UAF)
- Uses correct synchronization patterns
- Has been reviewed by the appropriate maintainer who also maintains
  stable trees
- Is self-contained with no feature additions

The lack of explicit stable tags appears to be an oversight given the
security nature of the bug and Greg KH's review. UAF vulnerabilities
typically warrant expedited backporting.

**YES**

 drivers/usb/gadget/function/f_fs.c | 53 ++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 47cfbe41fdff8..69f6e3c0f7e00 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -640,13 +640,22 @@ static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
 
 static int ffs_ep0_open(struct inode *inode, struct file *file)
 {
-	struct ffs_data *ffs = inode->i_private;
+	struct ffs_data *ffs = inode->i_sb->s_fs_info;
+	int ret;
 
-	if (ffs->state == FFS_CLOSING)
-		return -EBUSY;
+	/* Acquire mutex */
+	ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK);
+	if (ret < 0)
+		return ret;
 
-	file->private_data = ffs;
 	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);
 }
@@ -1193,14 +1202,33 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 static int
 ffs_epfile_open(struct inode *inode, struct file *file)
 {
-	struct ffs_epfile *epfile = inode->i_private;
+	struct ffs_data *ffs = inode->i_sb->s_fs_info;
+	struct ffs_epfile *epfile;
+	int ret;
 
-	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
+	/* Acquire mutex */
+	ret = ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK);
+	if (ret < 0)
+		return ret;
+
+	if (!atomic_inc_not_zero(&ffs->opened)) {
+		mutex_unlock(&ffs->mutex);
+		return -ENODEV;
+	}
+	/*
+	 * we want the state to be FFS_ACTIVE; FFS_ACTIVE alone is
+	 * not enough, though - we might have been through FFS_CLOSING
+	 * and back to FFS_ACTIVE, with our file already removed.
+	 */
+	epfile = smp_load_acquire(&inode->i_private);
+	if (unlikely(ffs->state != FFS_ACTIVE || !epfile)) {
+		mutex_unlock(&ffs->mutex);
+		ffs_data_closed(ffs);
 		return -ENODEV;
+	}
+	mutex_unlock(&ffs->mutex);
 
 	file->private_data = epfile;
-	ffs_data_opened(epfile->ffs);
-
 	return stream_open(inode, file);
 }
 
@@ -1332,7 +1360,7 @@ static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
 static int
 ffs_epfile_release(struct inode *inode, struct file *file)
 {
-	struct ffs_epfile *epfile = inode->i_private;
+	struct ffs_epfile *epfile = file->private_data;
 	struct ffs_dmabuf_priv *priv, *tmp;
 	struct ffs_data *ffs = epfile->ffs;
 
@@ -2352,6 +2380,11 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
 	return 0;
 }
 
+static void clear_one(struct dentry *dentry)
+{
+	smp_store_release(&dentry->d_inode->i_private, NULL);
+}
+
 static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 {
 	struct ffs_epfile *epfile = epfiles;
@@ -2359,7 +2392,7 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 	for (; count; --count, ++epfile) {
 		BUG_ON(mutex_is_locked(&epfile->mutex));
 		if (epfile->dentry) {
-			simple_recursive_removal(epfile->dentry, NULL);
+			simple_recursive_removal(epfile->dentry, clear_one);
 			epfile->dentry = NULL;
 		}
 	}
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2025-12-10  3:50 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-10  3:48 [PATCH AUTOSEL 6.18-6.17] functionfs: fix the open/removal races Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] scsi: qla2xxx: Use reinit_completion on mbx_intr_comp Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] via_wdt: fix critical boot hang due to unnamed resource allocation Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] ti-sysc: allow OMAP2 and OMAP4 timers to be reserved on AM33xx Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.15] scsi: qla2xxx: Fix lost interrupts with qlini_mode=disabled Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.12] exfat: zero out post-EOF page cache on file extension Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.12] scsi: smartpqi: Add support for Hurray Data new controller PCI device Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.12] scsi: ufs: host: mediatek: Fix shutdown/suspend race condition Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] powerpc/addnote: Fix overflow on 32-bit builds Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.6] fuse: Invalidate the page cache after FOPEN_DIRECT_IO write Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] ipmi: Fix __scan_channels() failing to rescan channels Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] scsi: qla2xxx: Fix initiator mode with qlini_mode=exclusive Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.17] um: init cpu_tasks[] earlier Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] reset: fix BIT macro reference Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] clk: mvebu: cp110 add CLK_IGNORE_UNUSED to pcie_x10, pcie_x11 & pcie_x4 Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.6] fuse: Always flush the page cache before FOPEN_DIRECT_IO write Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-5.10] firmware: imx: scu-irq: Init workqueue before request mbox channel Sasha Levin
2025-12-10  3:48 ` [PATCH AUTOSEL 6.18-6.17] scsi: lpfc: Fix reusing an ndlp that is marked NLP_DROPPED during FLOGI Sasha Levin
2025-12-10  3:49 ` [PATCH AUTOSEL 6.18-5.10] exfat: fix remount failure in different process environments Sasha Levin
2025-12-10  3:49 ` [PATCH AUTOSEL 6.18-5.10] ipmi: Fix the race between __scan_channels() and deliver_response() Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox