Re: [PATCH] [RFC] dma-buf: fix race condition between poll and close

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

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

[-- Attachment #2: 0001-epoll-fix-file-reference-counting.patch --]
[-- Type: text/x-patch, Size: 2019 bytes --]

From 718e10df61c522667cbb7c32978761cc3f1a4d3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig@amd.com>
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@amd.com>
---
 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