[PATCH] usb: f_fs: Fix use-after-free for epfile

[PATCH] usb: f_fs: Fix use-after-free for epfile

[Udipto Goswami]

From: Pratham Pratap <quic_ppratap@quicinc.com>

Consider a case where ffs_func_eps_disable is called from
ffs_func_disable as part of composition switch and at the
same time ffs_epfile_release get called from userspace.
ffs_epfile_release will free up the read buffer and call
ffs_data_closed which in turn destroys ffs->epfiles and
mark it as NULL. While this was happening the driver has
already initialized the local epfile in ffs_func_eps_disable
which is now freed and waiting to acquire the spinlock. Once
spinlock is acquired the driver proceeds with the stale value
of epfile and tries to free the already freed read buffer
causing use-after-free.

Following is the illustration of the race:

      CPU1                                  CPU2

   ffs_func_eps_disable
   epfiles (local copy)
					ffs_epfile_release
				  __ffs_epfile_read_buffer_free
					kfree(read_buffers)
					kfree(epfile)
(epfiles still accessible
 since local copy)
kfree(read_buffers) <use_after_free>

Another possibility of user after free is with the read_buffers
Currently, ffs_func_eps_disable & ffs_epfile_release can race,
if ffs_epfile_release ran in between while ffs_func_eps_disable
was executing, due to not being in any lock it can go ahead
and free the read buffer, but since ffs_func_eps_disable
maintains a local copy of epfiles, it will still be valid here
which when tried to free again will cause a user_after_free.
Following is the illustration of the case:
      CPU1				      CPU2

   ffs_func_eps_disable
   spin_lock_irqsave
   (epfile) local copy
	   				ffs_epfile_release
					__ffs_epfile_read_buffer_free
					kfree(epfile->read_buffer)
   __ffs_epfile_read_buffer_free
   kfree(epfile->read_buffer)
     <<use_after_free>>

Fix this races by taking epfile local copy & assigning it under
spinlock and if epfile(local) is null then update it in ffs->epfiles
then finally destroy it.

Change-Id: I85b1a0aea88c0033fbeef4c5db5104caac211540
Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
 drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 3c584da..3cdc636 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1268,10 +1268,13 @@ static int
 ffs_epfile_release(struct inode *inode, struct file *file)
 {
 	struct ffs_epfile *epfile = inode->i_private;
+	unsigned long flags;
 
 	ENTER();
 
+	spin_lock_irqsave(&epfile->ffs->eps_lock, flags);
 	__ffs_epfile_read_buffer_free(epfile);
+	spin_unlock_irqrestore(&epfile->ffs->eps_lock, flags);
 	ffs_data_closed(epfile->ffs);
 
 	return 0;
@@ -1711,16 +1714,23 @@ static void ffs_data_put(struct ffs_data *ffs)
 
 static void ffs_data_closed(struct ffs_data *ffs)
 {
+	struct ffs_epfile *epfile;
+	unsigned long flags;
 	ENTER();
 
 	if (atomic_dec_and_test(&ffs->opened)) {
 		if (ffs->no_disconnect) {
 			ffs->state = FFS_DEACTIVATED;
-			if (ffs->epfiles) {
-				ffs_epfiles_destroy(ffs->epfiles,
-						   ffs->eps_count);
-				ffs->epfiles = NULL;
-			}
+			spin_lock_irqsave(&ffs->eps_lock, flags);
+			epfile = ffs->epfiles;
+			ffs->epfiles = NULL;
+			spin_unlock_irqrestore(&ffs->eps_lock,
+							flags);
+
+			if (epfile)
+				ffs_epfiles_destroy(epfile,
+						 ffs->eps_count);
+
 			if (ffs->setup_state == FFS_SETUP_PENDING)
 				__ffs_ep0_stall(ffs);
 		} else {
@@ -1767,14 +1777,25 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
 
 static void ffs_data_clear(struct ffs_data *ffs)
 {
+	struct ffs_epfile *epfile;
+	unsigned long flags;
 	ENTER();
 
 	ffs_closed(ffs);
 
 	BUG_ON(ffs->gadget);
 
-	if (ffs->epfiles)
-		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
+	spin_lock_irqsave(&ffs->eps_lock, flags);
+	epfile = ffs->epfiles;
+	ffs->epfiles = NULL;
+	spin_unlock_irqrestore(&ffs->eps_lock, flags);
+	/* potential race possible between ffs_func_eps_disable
+	 * & ffs_epfile_release therefore maintaining a local
+	 * copy of epfile will save us from use-after-free.
+	 */
+	if (epfile)
+		ffs_epfiles_destroy(epfile,
+				    ffs->eps_count);
 
 	if (ffs->ffs_eventfd)
 		eventfd_ctx_put(ffs->ffs_eventfd);
@@ -1919,12 +1940,15 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 
 static void ffs_func_eps_disable(struct ffs_function *func)
 {
-	struct ffs_ep *ep         = func->eps;
-	struct ffs_epfile *epfile = func->ffs->epfiles;
-	unsigned count            = func->ffs->eps_count;
+	struct ffs_ep *ep;
+	struct ffs_epfile *epfile;
+	unsigned short count;
 	unsigned long flags;
 
 	spin_lock_irqsave(&func->ffs->eps_lock, flags);
+	count = func->ffs->eps_count;
+	epfile = func->ffs->epfiles;
+	ep = func->eps;
 	while (count--) {
 		/* pending requests get nuked */
 		if (ep->ep)
-- 
2.7.4

Re: [PATCH] usb: f_fs: Fix use-after-free for epfile

[Greg Kroah-Hartman]

On Fri, Nov 19, 2021 at 11:01:55AM +0530, Udipto Goswami wrote:
> From: Pratham Pratap <quic_ppratap@quicinc.com>
> 
> Consider a case where ffs_func_eps_disable is called from
> ffs_func_disable as part of composition switch and at the
> same time ffs_epfile_release get called from userspace.
> ffs_epfile_release will free up the read buffer and call
> ffs_data_closed which in turn destroys ffs->epfiles and
> mark it as NULL. While this was happening the driver has
> already initialized the local epfile in ffs_func_eps_disable
> which is now freed and waiting to acquire the spinlock. Once
> spinlock is acquired the driver proceeds with the stale value
> of epfile and tries to free the already freed read buffer
> causing use-after-free.
> 
> Following is the illustration of the race:
> 
>       CPU1                                  CPU2
> 
>    ffs_func_eps_disable
>    epfiles (local copy)
> 					ffs_epfile_release
> 				  __ffs_epfile_read_buffer_free
> 					kfree(read_buffers)
> 					kfree(epfile)
> (epfiles still accessible
>  since local copy)
> kfree(read_buffers) <use_after_free>
> 
> Another possibility of user after free is with the read_buffers
> Currently, ffs_func_eps_disable & ffs_epfile_release can race,
> if ffs_epfile_release ran in between while ffs_func_eps_disable
> was executing, due to not being in any lock it can go ahead
> and free the read buffer, but since ffs_func_eps_disable
> maintains a local copy of epfiles, it will still be valid here
> which when tried to free again will cause a user_after_free.
> Following is the illustration of the case:
>       CPU1				      CPU2
> 
>    ffs_func_eps_disable
>    spin_lock_irqsave
>    (epfile) local copy
> 	   				ffs_epfile_release
> 					__ffs_epfile_read_buffer_free
> 					kfree(epfile->read_buffer)
>    __ffs_epfile_read_buffer_free
>    kfree(epfile->read_buffer)
>      <<use_after_free>>
> 
> Fix this races by taking epfile local copy & assigning it under
> spinlock and if epfile(local) is null then update it in ffs->epfiles
> then finally destroy it.
> 
> Change-Id: I85b1a0aea88c0033fbeef4c5db5104caac211540

Always run scripts/checkpatch.pl on your changes so you do not get
grumpy maintainers asking you to run scripts/checkpatch.pl on your
changes.