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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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.