| Message ID | 1637299915-10477-1-git-send-email-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: f_fs: Fix use-after-free for epfile | expand |
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.
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)