On Fri, Nov 19, 2021 at 11:01:55AM +0530, Udipto Goswami wrote: > From: Pratham Pratap <quic_ppratap@xxxxxxxxxxx> > > 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.
