On 2020-08-04, Lokesh Gidra <lokeshgi...@google.com> wrote: > when get_unused_fd_flags returns error, ctx will be freed by > userfaultfd's release function, which is indirectly called by fput(). > Also, if anon_inode_getfile_secure() returns an error, then > userfaultfd_ctx_put() is called, which calls mmdrop() and frees ctx. > > Also, the O_CLOEXEC was inadvertently added to the call to > get_unused_fd_flags() [1].
I disagree that it is "wrong" to do O_CLOEXEC-by-default (after all, it's trivial to disable O_CLOEXEC, but it's non-trivial to enable it on an existing file descriptor because it's possible for another thread to exec() before you set the flag). Several new syscalls and fd-returning facilities are O_CLOEXEC-by-default now (the most obvious being pidfds and seccomp notifier fds). At the very least there should be a new flag added that sets O_CLOEXEC. > Adding Al Viro's suggested-by, based on [2]. > > [1] > https://lore.kernel.org/lkml/1f69c0ab-5791-974f-8bc0-3997ab1d6...@dancol.org/ > [2] https://lore.kernel.org/lkml/20200719165746.gj2786...@zeniv.linux.org.uk/ > > Fixes: d08ac70b1e0d (Wire UFFD up to SELinux) > Suggested-by: Al Viro <v...@zeniv.linux.org.uk> > Reported-by: syzbot+75867c44841cb6373...@syzkaller.appspotmail.com > Signed-off-by: Lokesh Gidra <lokeshgi...@google.com> > --- > fs/userfaultfd.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > index ae859161908f..e15eb8fdc083 100644 > --- a/fs/userfaultfd.c > +++ b/fs/userfaultfd.c > @@ -2042,24 +2042,18 @@ SYSCALL_DEFINE1(userfaultfd, int, flags) > O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS), > NULL); > if (IS_ERR(file)) { > - fd = PTR_ERR(file); > - goto out; > + userfaultfd_ctx_put(ctx); > + return PTR_ERR(file); > } > > - fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC); > + fd = get_unused_fd_flags(O_RDONLY); > if (fd < 0) { > fput(file); > - goto out; > + return fd; > } > > ctx->owner = file_inode(file); > fd_install(fd, file); > - > -out: > - if (fd < 0) { > - mmdrop(ctx->mm); > - kmem_cache_free(userfaultfd_ctx_cachep, ctx); > - } > return fd; > } > > -- > 2.28.0.163.g6104cc2f0b6-goog > -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
signature.asc
Description: PGP signature
