eventpoll: don't decrement ep refcount while still holding the ep mutex
Subsystems (2)
PATCH v2 1/9 eventpoll: don't decrement ep refcount while still holding the ep mutex
From: Linus Torvalds <[email protected]>
commit 8c2e52ebbe885c7eeaabd3b7ddcdc1246fc400d2 upstream.
Jann Horn points out that epoll is decrementing the ep refcount and then doing a
mutex_unlock(&ep->mtx);
afterwards. That's very wrong, because it can lead to a use-after-free.
That pattern is actually fine for the very last reference, because the code in question will delay the actual call to "ep_free(ep)" until after it has unlocked the mutex.
But it's wrong for the much subtler "next to last" case when somebody *else* may also be dropping their reference and free the ep while we're still using the mutex.
Note that this is true even if that other user is also using the same ep
mutex: mutexes, unlike spinlocks, can not be used for object ownership, even if they guarantee mutual exclusion.
A mutex "unlock" operation is not atomic, and as one user is still accessing the mutex as part of unlocking it, another user can come in and get the now released mutex and free the data structure while the first user is still cleaning up.
See our mutex documentation in Documentation/locking/mutex-design.rst, in particular the section [1] about semantics:
"mutex_unlock() may access the mutex structure even after it has
internally released the lock already - so it's not safe for another context to acquire the mutex and assume that the mutex_unlock() context is not using the structure anymore"
So if we drop our ep ref before the mutex unlock, but we weren't the last one, we may then unlock the mutex, another user comes in, drops _their_ reference and releases the 'ep' as it now has no users - all while the mutex_unlock() is still accessing it.
Fix this by simply moving the ep refcount dropping to outside the mutex:
the refcount itself is atomic, and doesn't need mutex protection (that's the whole _point_ of refcounts: unlike mutexes, they are inherently about object lifetimes).
--- fs/eventpoll.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
fs/eventpoll.c +5 -7
@@ -770,7 +770,7 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)call_rcu (&epi->rcu, epi_rcu_free);percpu_counter_dec (&ep->user->epoll_watches); -return ep_refcount_dec_and_test (ep); +return true; }/*
@@ -778,14 +778,14 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)*/ static void ep_remove_safe (struct eventpoll *ep,struct epitem *epi) { -WARN_ON_ONCE (__ep_remove (ep, epi, false)); +if (__ep_remove (ep, epi, false)) +WARN_ON_ONCE (ep_refcount_dec_and_test (ep)); }static void ep_clear_and_put (struct eventpoll *ep) {struct rb_node *rbp, *next;struct epitem *epi; -bool dispose;/* We need to release all tasks waiting for these file */ if (waitqueue_active (&ep->poll_wait))
@@ -818,10 +818,8 @@ static void ep_clear_and_put(struct eventpoll *ep)cond_resched (); } - dispose =ep_refcount_dec_and_test (ep);mutex_unlock (&ep->mtx); - -if (dispose) +if (ep_refcount_dec_and_test (ep))ep_free (ep); }
@@ -1001,7 +999,7 @@ void eventpoll_release_file(struct file *file) dispose =__ep_remove (ep, epi, true);mutex_unlock (&ep->mtx); -if (dispose) +if (dispose &&ep_refcount_dec_and_test (ep))ep_free (ep); goto again; }
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 2/9 file: add fput() cleanup helper
From: Christian Brauner <[email protected]>
[ Upstream commit 257b1c2c78c25643526609dee0c15f1544eb3252 ]
Add a simple helper to put a file reference.
--- include/linux/file.h | 2 ++ 1 file changed, 2 insertions(+)
include/linux/file.h +2 -0
@@ -11,6 +11,7 @@#include <linux/posix_types.h> #include <linux/errno.h> #include <linux/cleanup.h> +#include <linux/err.h> struct file ;
@@ -95,6 +96,7 @@ extern void put_unused_fd(unsigned int fd); DEFINE_CLASS(get_unused_fd, int, if (_T >= 0) put_unused_fd(_T), get_unused_fd_flags(flags), unsigned flags) +DEFINE_FREE(fput, struct file *, if (!IS_ERR_OR_NULL(_T)) fput(_T))extern void fd_install (unsigned int fd,struct file *file);
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 3/9 eventpoll: use hlist_is_singular_node() in __ep_remove()
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit 3d9fd0abc94d8cd430cc7cd7d37ce5e5aae2cd2b ]
Replace the open-coded "epi is the only entry in file->f_ep" check with hlist_is_singular_node(). Same semantics, and the helper avoids the head-cacheline access in the common false case.
Stable-dep-of: a6dc643c6931 ("eventpoll: fix ep_remove struct eventpoll / struct file UAF")
--- fs/eventpoll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/eventpoll.c +1 -1
@@ -738,7 +738,7 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force) to_free =NULL ; head = file->f_ep; -if (head->first == &epi->fllink && !epi->fllink.next) { +if (hlist_is_singular_node (&epi->fllink, head)) {/* See eventpoll_release() for details. */ WRITE_ONCE (file->f_ep,NULL );if (!is_file_epoll (file)) {
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 4/9 eventpoll: split __ep_remove()
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit 0f7bdfd413000985de09fc39eb9efa1e091a3ce0 ]
Split __ep_remove() to delineate file removal from epoll item removal.
Stable-dep-of: a6dc643c6931 ("eventpoll: fix ep_remove struct eventpoll / struct file UAF")
--- fs/eventpoll.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)
fs/eventpoll.c +23 -4
@@ -708,6 +708,9 @@ static void ep_free(struct eventpoll *ep)kfree_rcu (ep, rcu); } +static void __ep_remove_file (struct eventpoll *ep,struct epitem *epi,struct file *file); +static bool __ep_remove_epi (struct eventpoll *ep,struct epitem *epi); +/* * Removes a "struct epitem" from the eventpoll RB tree and deallocates * all the associated resources. Must be called with "mtx" held.
@@ -719,8 +722,6 @@ static void ep_free(struct eventpoll *ep)static bool __ep_remove (struct eventpoll *ep,struct epitem *epi,bool force) {struct file *file = epi->ffd.file; -struct epitems_head *to_free; -struct hlist_head *head;lockdep_assert_irqs_enabled ();
@@ -736,8 +737,21 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)return false; } - to_free =NULL ; - head = file->f_ep; +__ep_remove_file (ep, epi, file); +return __ep_remove_epi (ep, epi); +} + +/* +* Called with &file->f_lock held, +* returns with it released +*/ +static void __ep_remove_file (struct eventpoll *ep,struct epitem *epi,struct file *file) +{ +struct epitems_head *to_free =NULL ; +struct hlist_head *head = file->f_ep; + +lockdep_assert_held (&ep->mtx); +if (hlist_is_singular_node (&epi->fllink, head)) {/* See eventpoll_release() for details. */ WRITE_ONCE (file->f_ep,NULL );
@@ -751,6 +765,11 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)hlist_del_rcu (&epi->fllink);spin_unlock (&file->f_lock);free_ephead (to_free); +} + +static bool __ep_remove_epi (struct eventpoll *ep,struct epitem *epi) +{ +lockdep_assert_held (&ep->mtx);rb_erase_cached (&epi->rbn, &ep->rbr);
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 5/9 eventpoll: kill __ep_remove()
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit e9e5cd40d7c403e19f21d0f7b8b8ba3a76b58330 ]
Remove the boolean conditional in __ep_remove() and restructure the code so the check for racing with eventpoll_release_file() are only done in the ep_remove_safe() path where they belong.
Stable-dep-of: a6dc643c6931 ("eventpoll: fix ep_remove struct eventpoll / struct file UAF")
--- fs/eventpoll.c | 67 ++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 37 deletions(-)
fs/eventpoll.c +30 -37
@@ -708,49 +708,18 @@ static void ep_free(struct eventpoll *ep)kfree_rcu (ep, rcu); } -static void __ep_remove_file (struct eventpoll *ep,struct epitem *epi,struct file *file); -static bool __ep_remove_epi (struct eventpoll *ep,struct epitem *epi); - -/* -* Removes a "struct epitem" from the eventpoll RB tree and deallocates -* all the associated resources. Must be called with "mtx" held. -* If the dying flag is set, do the removal only if force is true. -* This prevents ep_clear_and_put() from dropping all the ep references -* while running concurrently with eventpoll_release_file(). -* Returns true if the eventpoll can be disposed. -*/ -static bool __ep_remove (struct eventpoll *ep,struct epitem *epi,bool force) -{ -struct file *file = epi->ffd.file; - -lockdep_assert_irqs_enabled (); - -/* -* Removes poll wait queue hooks. -*/ -ep_unregister_pollwait (ep, epi); - -/* Remove the current item from the list of epoll hooks */ -spin_lock (&file->f_lock); -if (epi->dying && !force) { -spin_unlock (&file->f_lock); -return false; - } - -__ep_remove_file (ep, epi, file); -return __ep_remove_epi (ep, epi); -} -/* * Called with &file->f_lock held, * returns with it released */ -static void __ep_remove_file (struct eventpoll *ep,struct epitem *epi,struct file *file) +static void __ep_remove_file (struct eventpoll *ep,struct epitem *epi, +struct file *file) {struct epitems_head *to_free =NULL ;struct hlist_head *head = file->f_ep;lockdep_assert_held (&ep->mtx); +lockdep_assert_held (&file->f_lock);if (hlist_is_singular_node (&epi->fllink, head)) {/* See eventpoll_release() for details. */
@@ -797,7 +766,25 @@ static bool __ep_remove_epi(struct eventpoll *ep, struct epitem *epi)*/ static void ep_remove_safe (struct eventpoll *ep,struct epitem *epi) { -if (__ep_remove (ep, epi, false)) +struct file *file = epi->ffd.file; + +lockdep_assert_irqs_enabled (); +lockdep_assert_held (&ep->mtx); + +ep_unregister_pollwait (ep, epi); + +/* sync with eventpoll_release_file() */ +if (unlikely (READ_ONCE (epi->dying))) +return ; + +spin_lock (&file->f_lock); +if (epi->dying) { +spin_unlock (&file->f_lock); +return ; + } +__ep_remove_file (ep, epi, file); + +if (__ep_remove_epi (ep, epi))WARN_ON_ONCE (ep_refcount_dec_and_test (ep)); }
@@ -1006,7 +993,7 @@ void eventpoll_release_file(struct file *file)spin_lock (&file->f_lock);if (file->f_ep && file->f_ep->first) { epi = hlist_entry(file->f_ep->first, struct epitem, fllink); - epi->dying = true; +WRITE_ONCE (epi->dying, true);spin_unlock (&file->f_lock);/*
@@ -1015,7 +1002,13 @@ void eventpoll_release_file(struct file *file)*/ ep = epi->ep;mutex_lock (&ep->mtx); - dispose =__ep_remove (ep, epi, true); + +ep_unregister_pollwait (ep, epi); + +spin_lock (&file->f_lock); +__ep_remove_file (ep, epi, file); + dispose =__ep_remove_epi (ep, epi); +mutex_unlock (&ep->mtx);if (dispose &&ep_refcount_dec_and_test (ep))
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 6/9 eventpoll: drop vestigial __ prefix from ep_remove_{file,epi}()
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit 0feaf644f7180c4a91b6b405a881afbfd958f1cf ]
With __ep_remove() gone, the double-underscore on __ep_remove_file() and __ep_remove_epi() no longer contrasts with a __-less parent and just reads as noise. Rename both to ep_remove_file() and ep_remove_epi(). No functional change.
Stable-dep-of: a6dc643c6931 ("eventpoll: fix ep_remove struct eventpoll / struct file UAF")
--- fs/eventpoll.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
fs/eventpoll.c +6 -6
@@ -712,7 +712,7 @@ static void ep_free(struct eventpoll *ep)* Called with &file->f_lock held, * returns with it released */ -static void __ep_remove_file (struct eventpoll *ep,struct epitem *epi, +static void ep_remove_file (struct eventpoll *ep,struct epitem *epi,struct file *file) {struct epitems_head *to_free =NULL ;
@@ -736,7 +736,7 @@ static void __ep_remove_file(struct eventpoll *ep, struct epitem *epi,free_ephead (to_free); } -static bool __ep_remove_epi (struct eventpoll *ep,struct epitem *epi) +static bool ep_remove_epi (struct eventpoll *ep,struct epitem *epi) {lockdep_assert_held (&ep->mtx);
@@ -782,9 +782,9 @@ static void ep_remove_safe(struct eventpoll *ep, struct epitem *epi)spin_unlock (&file->f_lock);return ; } -__ep_remove_file (ep, epi, file); +ep_remove_file (ep, epi, file); -if (__ep_remove_epi (ep, epi)) +if (ep_remove_epi (ep, epi))WARN_ON_ONCE (ep_refcount_dec_and_test (ep)); }
@@ -1006,8 +1006,8 @@ void eventpoll_release_file(struct file *file)ep_unregister_pollwait (ep, epi);spin_lock (&file->f_lock); -__ep_remove_file (ep, epi, file); - dispose =__ep_remove_epi (ep, epi); +ep_remove_file (ep, epi, file); + dispose =ep_remove_epi (ep, epi);mutex_unlock (&ep->mtx);
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 7/9 eventpoll: rename ep_remove_safe() back to ep_remove()
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit 0bade234723e40e4937be912e105785d6a51464e ]
The current name is just confusing and doesn't clarify anything.
Stable-dep-of: a6dc643c6931 ("eventpoll: fix ep_remove struct eventpoll / struct file UAF")
--- fs/eventpoll.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
fs/eventpoll.c +8 -8
@@ -764,7 +764,7 @@ static bool ep_remove_epi(struct eventpoll *ep, struct epitem *epi)/* * ep_remove variant for callers owing an additional reference to the ep */ -static void ep_remove_safe (struct eventpoll *ep,struct epitem *epi) +static void ep_remove (struct eventpoll *ep,struct epitem *epi) {struct file *file = epi->ffd.file;
@@ -811,7 +811,7 @@ static void ep_clear_and_put(struct eventpoll *ep)/* * Walks through the whole tree and try to free each "struct epitem". -* Note that ep_remove_safe() will not remove the epitem in case of a +* Note that ep_remove() will not remove the epitem in case of a * racing eventpoll_release_file(); the latter will do the removal. * At this point we are sure no poll callbacks will be lingering around. * Since we still own a reference to the eventpoll struct, the loop can't
@@ -820,7 +820,7 @@ static void ep_clear_and_put(struct eventpoll *ep)for (rbp =rb_first_cached (&ep->rbr); rbp; rbp = next) { next =rb_next (rbp); epi = rb_entry(rbp, struct epitem, rbn); -ep_remove_safe (ep, epi); +ep_remove (ep, epi);cond_resched (); }
@@ -1585,21 +1585,21 @@ static int ep_insert(struct eventpoll *ep, const struct epoll_event *event,mutex_unlock (&tep->mtx);/* -* ep_remove_safe() calls in the later error paths can't lead to +* ep_remove() calls in the later error paths can't lead to * ep_free() as the ep file itself still holds an ep reference. */ ep_get (ep);/* now check if we've created too many backpaths */ if (unlikely (full_check &&reverse_path_check ())) { -ep_remove_safe (ep, epi); +ep_remove (ep, epi);return -EINVAL ; }if (epi->event.events &EPOLLWAKEUP ) { error =ep_create_wakeup_source (epi);if (error) { -ep_remove_safe (ep, epi); +ep_remove (ep, epi);return error; } }
@@ -1623,7 +1623,7 @@ static int ep_insert(struct eventpoll *ep, const struct epoll_event *event,* high memory pressure. */ if (unlikely (!epq.epi)) { -ep_remove_safe (ep, epi); +ep_remove (ep, epi);return -ENOMEM ; }
@@ -2302,7 +2302,7 @@ int do_epoll_ctl(int epfd, int op, int fd, struct epoll_event *epds,* The eventpoll itself is still alive: the refcount * can't go to zero here. */ -ep_remove_safe (ep, epi); +ep_remove (ep, epi); error =0 ; }else { error = -ENOENT ;
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 8/9 eventpoll: move epi_fget() up
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit 86e87059e6d1fd5115a31949726450ed03c1073b ]
We'll need it when removing files so move it up. No functional change.
Stable-dep-of: a6dc643c6931 ("eventpoll: fix ep_remove struct eventpoll / struct file UAF") [file_ref_get(&file->f_ref) from original commit left as
atomic_long_inc_not_zero(&file->f_count) due to v6.12.y missing commit 90ee6ed776c0 ("fs: port files to file_ref") and its dependent commit 08ef26ea9ab3 ("fs: add file_ref")]
--- fs/eventpoll.c | 56 +++++++++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 28 deletions(-)
fs/eventpoll.c +28 -28
@@ -708,6 +708,34 @@ static void ep_free(struct eventpoll *ep)kfree_rcu (ep, rcu); } +/* +* The ffd.file pointer may be in the process of being torn down due to +* being closed, but we may not have finished eventpoll_release() yet. +* +* Normally, even with the atomic_long_inc_not_zero, the file may have +* been free'd and then gotten re-allocated to something else (since +* files are not RCU-delayed, they are SLAB_TYPESAFE_BY_RCU). +* +* But for epoll, users hold the ep->mtx mutex, and as such any file in +* the process of being free'd will block in eventpoll_release_file() +* and thus the underlying file allocation will not be free'd, and the +* file re-use cannot happen. +* +* For the same reason we can avoid a rcu_read_lock() around the +* operation - 'ffd.file' cannot go away even if the refcount has +* reached zero (but we must still not call out to ->poll() functions +* etc). +*/ +static struct file *epi_fget (const struct epitem *epi) +{ +struct file *file; + + file = epi->ffd.file; +if (!atomic_long_inc_not_zero (&file->f_count)) + file =NULL ; +return file; +} +/* * Called with &file->f_lock held, * returns with it released
@@ -879,34 +907,6 @@ static __poll_t __ep_eventpoll_poll(struct file *file, poll_table *wait, int depreturn res; } -/* -* The ffd.file pointer may be in the process of being torn down due to -* being closed, but we may not have finished eventpoll_release() yet. -* -* Normally, even with the atomic_long_inc_not_zero, the file may have -* been free'd and then gotten re-allocated to something else (since -* files are not RCU-delayed, they are SLAB_TYPESAFE_BY_RCU). -* -* But for epoll, users hold the ep->mtx mutex, and as such any file in -* the process of being free'd will block in eventpoll_release_file() -* and thus the underlying file allocation will not be free'd, and the -* file re-use cannot happen. -* -* For the same reason we can avoid a rcu_read_lock() around the -* operation - 'ffd.file' cannot go away even if the refcount has -* reached zero (but we must still not call out to ->poll() functions -* etc). -*/ -static struct file *epi_fget (const struct epitem *epi) -{ -struct file *file; - - file = epi->ffd.file; -if (!atomic_long_inc_not_zero (&file->f_count)) - file =NULL ; -return file; -} -/* * Differs from ep_eventpoll_poll() in that internal callers already have * the ep->mtx so we need to start from depth=1, such that mutex_lock_nested()
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
PATCH v2 9/9 eventpoll: fix ep_remove struct eventpoll / struct file UAF
In reply to Lee JonesFrom: Christian Brauner <[email protected]>
[ Upstream commit a6dc643c69311677c574a0f17a3f4d66a5f3744b ]
ep_remove() (via ep_remove_file()) cleared file->f_ep under file->f_lock but then kept using @file inside the critical section (is_file_epoll(), hlist_del_rcu() through the head, spin_unlock). A concurrent __fput() taking the eventpoll_release() fastpath in that window observed the transient NULL, skipped eventpoll_release_file() and ran to f_op->release / file_free().
For the epoll-watches-epoll case, f_op->release is ep_eventpoll_release() -> ep_clear_and_put() -> ep_free(), which kfree()s the watched struct eventpoll. Its embedded ->refs hlist_head is exactly where epi->fllink.pprev points, so the subsequent hlist_del_rcu()'s "*pprev = next" scribbles into freed kmalloc-192 memory.
In addition, struct file is SLAB_TYPESAFE_BY_RCU, so the slot backing @file could be recycled by alloc_empty_file() -- reinitializing f_lock and f_ep -- while ep_remove() is still nominally inside that lock. The upshot is an attacker-controllable kmem_cache_free() against the wrong slab cache.
Pin @file via epi_fget() at the top of ep_remove() and gate the critical section on the pin succeeding. With the pin held @file cannot reach refcount zero, which holds __fput() off and transitively keeps the watched struct eventpoll alive across the hlist_del_rcu() and the f_lock use, closing both UAFs.
If the pin fails @file has already reached refcount zero and its __fput() is in flight. Because we bailed before clearing f_ep, that path takes the eventpoll_release() slow path into eventpoll_release_file() and blocks on ep->mtx until the waiter side's ep_clear_and_put() drops it. The bailed epi's share of ep->refcount stays intact, so the trailing ep_refcount_dec_and_test() in ep_clear_and_put() cannot free the eventpoll out from under eventpoll_release_file(); the orphaned epi is then cleaned up there.
A successful pin also proves we are not racing eventpoll_release_file() on this epi, so drop the now-redundant re-check of epi->dying under f_lock. The cheap lockless READ_ONCE(epi->dying) fast-path bailout stays.
--- fs/eventpoll.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
fs/eventpoll.c +10 -6
@@ -794,22 +794,26 @@ static bool ep_remove_epi(struct eventpoll *ep, struct epitem *epi)*/ static void ep_remove (struct eventpoll *ep,struct epitem *epi) { -struct file *file = epi->ffd.file; + struct file *file __free(fput) = NULL;lockdep_assert_irqs_enabled ();lockdep_assert_held (&ep->mtx);ep_unregister_pollwait (ep, epi); -/* sync with eventpoll_release_file() */ +/* cheap sync with eventpoll_release_file() */ if (unlikely (READ_ONCE (epi->dying)))return ; -spin_lock (&file->f_lock); -if (epi->dying) { -spin_unlock (&file->f_lock); +/* +* If we manage to grab a reference it means we're not in +* eventpoll_release_file() and aren't going to be. +*/ + file =epi_fget (epi); +if (!file)return ; - } + +spin_lock (&file->f_lock);ep_remove_file (ep, epi, file);if (ep_remove_epi (ep, epi))
Signature · 1 line
2.56.0.rc1.315.gc6ed9934b7-goog
Quoted 1 line
commit
8c2e52ebbe885c7eeaabd3b7ddcdc1246fc400d2upstream.
Queued the series for 5.15, thanks.
Signature · 2 lines
Thanks, Sasha