Am 03.05.24 um 09:07 schrieb Dmitry Antipov:
On 4/24/24 2:28 PM, Christian König wrote:
I don't fully understand how that happens either, it could be that
there is some bug in the EPOLL_FD code. Maybe it's a race when the
EPOLL file descriptor is closed or something like that.
IIUC the race condition looks like the following:
Thread 0 Thread 1
-> do_epoll_ctl()
f_count++, now 2
...
... -> vfs_poll(), f_count == 2
... ...
<- do_epoll_ctl() ...
f_count--, now 1 ...
-> filp_close(), f_count == 1 ...
... -> dma_buf_poll(), f_count == 1
-> fput() ... [*** race window ***]
f_count--, now 0 -> maybe get_file(), now ???
-> __fput() (delayed)
E.g. dma_buf_poll() may be entered in thread 1 with f->count == 1
and call to get_file() shortly later (and may even skip this if
there is nothing to EPOLLIN or EPOLLOUT). During this time window,
thread 0 may call fput() (on behalf of close() in this example)
and (since it sees f->count == 1) file is scheduled to delayed_fput().
Wow, this is indeed looks like a bug in the epoll implementation.
Basically Thread 1 needs to make sure that the file reference can't
vanish. Otherwise it is illegal to call vfs_poll().
I only skimmed over the epoll implementation and never looked at the
code before, but of hand it looks like the implementation uses a mutex
in the eventpoll structure which makes sure that the epitem structures
don't go away during the vfs_poll() call.
But when I look closer at the do_epoll_ctl() function I can see that the
file reference acquired isn't handed over to the epitem structure, but
rather dropped when returning from the function.
That seems to be a buggy behavior because it means that vfs_poll() can
be called with a stale file pointer. That in turn can lead to all kind
of use after free bugs.
Attached is a compile only tested patch, please verify if it fixes your
problem.
Regards,
Christian.
Dmitry
From 718e10df61c522667cbb7c32978761cc3f1a4d3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig@xxxxxxx>
Date: Fri, 3 May 2024 10:01:00 +0200
Subject: [PATCH] epoll: fix file reference counting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The epoll implementation keeps pointers to struct files and
eventually calls vfs_poll() on them without holding a reference.
This can result in various use after free issues when the file
descriptor which is added to an epoll_fd is closed without
previously removing it from the epoll_fd.
Try to fix this by getting and dropping the reference to the file
pointer as necessary.
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
fs/eventpoll.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 882b89edc52a..f3584f6fbaed 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -349,10 +349,17 @@ static inline int is_file_epoll(struct file *f)
static inline void ep_set_ffd(struct epoll_filefd *ffd,
struct file *file, int fd)
{
- ffd->file = file;
+ ffd->file = get_file(file);
ffd->fd = fd;
}
+/* Cleanup the structure used as key for the RB tree */
+static inline void ep_clear_ffd(struct epoll_filefd *ffd)
+{
+ fput(ffd->file);
+ ffd->file = NULL;
+}
+
/* Compare RB tree keys */
static inline int ep_cmp_ffd(struct epoll_filefd *p1,
struct epoll_filefd *p2)
@@ -843,6 +850,8 @@ static bool __ep_remove(struct eventpoll *ep, struct epitem *epi, bool force)
write_unlock_irq(&ep->lock);
wakeup_source_unregister(ep_wakeup_source(epi));
+ ep_clear_ffd(&epi->ffd);
+
/*
* At this point it is safe to free the eventpoll item. Use the union
* field epi->rcu, since we are trying to minimize the size of
@@ -1126,6 +1135,7 @@ static struct epitem *ep_find(struct eventpoll *ep, struct file *file, int fd)
break;
}
}
+ ep_clear_ffd(&ffd);
return epir;
}
--
2.34.1
