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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux