Skip to contentSkip to conversation

usb: gadget: f_fs: fix use-after-free on umount

PATCH In review Eduard Zateev 3 participants 3 messages Oct 8 – Oct 14 USB

lore
Share & tools
lore

Eduard Zateev

author
#0 lore raw

PATCH 1/1 usb: gadget: f_fs: fix use-after-free on umount

ffs_free_inst() releases a device and drops ffs_dev_lock before it removes and frees the device. A concurrent FunctionFS mount can find the released device in that window, attach a new ffs_data to it, and then use the freed device from ffs_closed() during unmount.

Keep ffs_dev_lock held across release and free so a new mount cannot attach to a device being torn down. In ffs_data_put(), read ffs->private_data only after acquiring the same lock, since a concurrent ffs_free_inst() may clear and free the referenced device while the putter waits for the lock.

Fixes ecfbd7b9054b ("usb: gadget: f_fs: Fix setting of device and driver data cross-references")
Reported-by [email protected]
Link https://syzkaller.appspot.com/bug?extid=6227549bd2c8a1ec8ba0
Signed-off-by Eduard Zateev <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
drivers/usb/gadget/function/f_fs.c +14 -7
index c64a268e9..791425390 100644
@@ -288,7 +288,7 @@ static struct ffs_dev *_ffs_find_dev(const char *name);
 static struct ffs_dev *_ffs_alloc_dev(void);
 static void _ffs_free_dev(struct ffs_dev *dev);
 static int ffs_acquire_dev(const char *dev_name, struct ffs_data *ffs_data);
-static void ffs_release_dev(struct ffs_dev *ffs_dev);
+static void _ffs_release_dev(struct ffs_dev *ffs_dev);
 static int ffs_ready(struct ffs_data *ffs);
 static void ffs_closed(struct ffs_data *ffs);
 static void ffs_reset_work(struct work_struct *work);

@@ -2231,7 +2231,14 @@ static void ffs_data_put(struct ffs_data *ffs)
 	if (refcount_dec_and_test(&ffs->ref)) {
 		pr_info("%s(): freeing\n", __func__);
 		ffs_data_clear(ffs);
-		ffs_release_dev(ffs->private_data);
+		/*
+		 * ffs->private_data must be re-read under ffs_dev_lock:
+		 * a concurrent ffs_free_inst() may have NULLed it and
+		 * freed the dev it pointed to.
+		 */
+		ffs_dev_lock();
+		_ffs_release_dev(ffs->private_data);
+		ffs_dev_unlock();
 		BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
 		       swait_active(&ffs->ep0req_completion.wait) ||
 		       waitqueue_active(&ffs->wait));

@@ -4147,8 +4154,8 @@ 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);
 	ffs_dev_lock();
+	_ffs_release_dev(opts->dev);
 	_ffs_free_dev(opts->dev);
 	ffs_dev_unlock();
 	kfree(opts);

@@ -4363,10 +4370,11 @@ static int ffs_acquire_dev(const char *dev_name, struct ffs_data *ffs_data)
 	return ret;
 }
 
-static void ffs_release_dev(struct ffs_dev *ffs_dev)
+/*
+ * ffs_dev_lock must be taken by the caller
+ */
+static void _ffs_release_dev(struct ffs_dev *ffs_dev)
 {
-	ffs_dev_lock();
-
 	if (ffs_dev && ffs_dev->mounted) {
 		ffs_dev->mounted = false;
 		if (ffs_dev->ffs_data) {

@@ -4378,7 +4386,6 @@ static void ffs_release_dev(struct ffs_dev *ffs_dev)
 			ffs_dev->ffs_release_dev_callback(ffs_dev);
 	}
 
-	ffs_dev_unlock();
 }
 
 static int ffs_ready(struct ffs_data *ffs)
Signature · 1 line
2.56.0
AI review
#1 lore raw
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 11 lines

ffs_free_inst() releases a device and drops ffs_dev_lock before it removes and frees the device. A concurrent FunctionFS mount can find the released device in that window, attach a new ffs_data to it, and then use the freed device from ffs_closed() during unmount.

Keep ffs_dev_lock held across release and free so a new mount cannot attach to a device being torn down. In ffs_data_put(), read ffs->private_data only after acquiring the same lock, since a concurrent ffs_free_inst() may clear and free the referenced device while the putter waits for the lock.

Quoted 6 lines
Fixes ecfbd7b9054b ("usb: gadget: f_fs: Fix setting of device and driver data cross-references")
Reported-by [email protected]
Link https://syzkaller.appspot.com/bug?extid=6227549bd2c8a1ec8ba0
Signed-off-by Eduard Zateev <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1