Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
@ 2020-08-04 20:31 Lokesh Gidra
  2020-08-04 20:45 ` Eric Biggers
  2020-08-05  3:47 ` Aleksa Sarai
  0 siblings, 2 replies; 9+ messages in thread
From: Lokesh Gidra @ 2020-08-04 20:31 UTC (permalink / raw)
  To: viro, stephen.smalley.work, casey, jmorris
  Cc: kaleshsingh, dancol, surenb, linux-fsdevel, linux-kernel, nnk,
	jeffv, calin, kernel-team, yanfei.xu, Lokesh Gidra,
	syzbot+75867c44841cb6373570

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].

Adding Al Viro's suggested-by, based on [2].

[1] https://lore.kernel.org/lkml/1f69c0ab-5791-974f-8bc0-3997ab1d61ea@dancol.org/
[2] https://lore.kernel.org/lkml/20200719165746.GJ2786714@ZenIV.linux.org.uk/

Fixes: d08ac70b1e0d (Wire UFFD up to SELinux)
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Reported-by: syzbot+75867c44841cb6373570@syzkaller.appspotmail.com
Signed-off-by: Lokesh Gidra <lokeshgidra@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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-04 20:31 [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC Lokesh Gidra
@ 2020-08-04 20:45 ` Eric Biggers
  2020-08-04 20:49   ` Lokesh Gidra
  2020-08-05  3:47 ` Aleksa Sarai
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2020-08-04 20:45 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: viro, stephen.smalley.work, casey, jmorris, kaleshsingh, dancol,
	surenb, linux-fsdevel, linux-kernel, nnk, jeffv, calin,
	kernel-team, yanfei.xu, syzbot+75867c44841cb6373570

On Tue, Aug 04, 2020 at 01:31:55PM -0700, Lokesh Gidra 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].
> 
> Adding Al Viro's suggested-by, based on [2].
> 
> [1] https://lore.kernel.org/lkml/1f69c0ab-5791-974f-8bc0-3997ab1d61ea@dancol.org/
> [2] https://lore.kernel.org/lkml/20200719165746.GJ2786714@ZenIV.linux.org.uk/
> 
> Fixes: d08ac70b1e0d (Wire UFFD up to SELinux)
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Reported-by: syzbot+75867c44841cb6373570@syzkaller.appspotmail.com
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>

What branch does this patch apply to?  Neither mainline nor linux-next works.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-04 20:45 ` Eric Biggers
@ 2020-08-04 20:49   ` Lokesh Gidra
  2020-08-04 20:58     ` Eric Biggers
  0 siblings, 1 reply; 9+ messages in thread
From: Lokesh Gidra @ 2020-08-04 20:49 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Alexander Viro, Stephen Smalley, casey, James Morris,
	Kalesh Singh, Daniel Colascione, Suren Baghdasaryan,
	Linux FS Devel, linux-kernel, Nick Kralevich,
	Jeffrey Vander Stoep, Calin Juravle, kernel-team, yanfei.xu,
	syzbot+75867c44841cb6373570

On Tue, Aug 4, 2020 at 1:45 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Aug 04, 2020 at 01:31:55PM -0700, Lokesh Gidra 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].
> >
> > Adding Al Viro's suggested-by, based on [2].
> >
> > [1] https://lore.kernel.org/lkml/1f69c0ab-5791-974f-8bc0-3997ab1d61ea@dancol.org/
> > [2] https://lore.kernel.org/lkml/20200719165746.GJ2786714@ZenIV.linux.org.uk/
> >
> > Fixes: d08ac70b1e0d (Wire UFFD up to SELinux)
> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > Reported-by: syzbot+75867c44841cb6373570@syzkaller.appspotmail.com
> > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
>
> What branch does this patch apply to?  Neither mainline nor linux-next works.
>
On James Morris' tree (secure_uffd_v5.9 branch).

> - Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-04 20:49   ` Lokesh Gidra
@ 2020-08-04 20:58     ` Eric Biggers
  2020-08-04 23:34       ` Lokesh Gidra
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2020-08-04 20:58 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: Alexander Viro, Stephen Smalley, casey, James Morris,
	Kalesh Singh, Daniel Colascione, Suren Baghdasaryan,
	Linux FS Devel, linux-kernel, Nick Kralevich,
	Jeffrey Vander Stoep, Calin Juravle, kernel-team, yanfei.xu,
	syzbot+75867c44841cb6373570

On Tue, Aug 04, 2020 at 01:49:30PM -0700, Lokesh Gidra wrote:
> On Tue, Aug 4, 2020 at 1:45 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Tue, Aug 04, 2020 at 01:31:55PM -0700, Lokesh Gidra 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].
> > >
> > > Adding Al Viro's suggested-by, based on [2].
> > >
> > > [1] https://lore.kernel.org/lkml/1f69c0ab-5791-974f-8bc0-3997ab1d61ea@dancol.org/
> > > [2] https://lore.kernel.org/lkml/20200719165746.GJ2786714@ZenIV.linux.org.uk/
> > >
> > > Fixes: d08ac70b1e0d (Wire UFFD up to SELinux)
> > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > > Reported-by: syzbot+75867c44841cb6373570@syzkaller.appspotmail.com
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> >
> > What branch does this patch apply to?  Neither mainline nor linux-next works.
> >
> On James Morris' tree (secure_uffd_v5.9 branch).
> 

For those of us not "in the know", that apparently means branch secure_uffd_v5.9
of https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git

Perhaps it would make more sense to resend your original patch series with this
fix folded in?

> 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;

This introduces the opposite bug: now it's hardcoded to *not* use O_CLOEXEC,
instead of using the flag the user passed in the flags argument to the syscall.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-04 20:58     ` Eric Biggers
@ 2020-08-04 23:34       ` Lokesh Gidra
  0 siblings, 0 replies; 9+ messages in thread
From: Lokesh Gidra @ 2020-08-04 23:34 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Alexander Viro, Stephen Smalley, casey, James Morris,
	Kalesh Singh, Daniel Colascione, Suren Baghdasaryan,
	Linux FS Devel, linux-kernel, Nick Kralevich,
	Jeffrey Vander Stoep, Calin Juravle, kernel-team, yanfei.xu,
	syzbot+75867c44841cb6373570

On Tue, Aug 4, 2020 at 1:58 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Aug 04, 2020 at 01:49:30PM -0700, Lokesh Gidra wrote:
> > On Tue, Aug 4, 2020 at 1:45 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Tue, Aug 04, 2020 at 01:31:55PM -0700, Lokesh Gidra 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].
> > > >
> > > > Adding Al Viro's suggested-by, based on [2].
> > > >
> > > > [1] https://lore.kernel.org/lkml/1f69c0ab-5791-974f-8bc0-3997ab1d61ea@dancol.org/
> > > > [2] https://lore.kernel.org/lkml/20200719165746.GJ2786714@ZenIV.linux.org.uk/
> > > >
> > > > Fixes: d08ac70b1e0d (Wire UFFD up to SELinux)
> > > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > Reported-by: syzbot+75867c44841cb6373570@syzkaller.appspotmail.com
> > > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > >
> > > What branch does this patch apply to?  Neither mainline nor linux-next works.
> > >
> > On James Morris' tree (secure_uffd_v5.9 branch).
> >
>
> For those of us not "in the know", that apparently means branch secure_uffd_v5.9
> of https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
>
> Perhaps it would make more sense to resend your original patch series with this
> fix folded in?
>
OK. I'll resend the whole patch series with the fixes soon.

> > 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;
>
> This introduces the opposite bug: now it's hardcoded to *not* use O_CLOEXEC,
> instead of using the flag the user passed in the flags argument to the syscall.

I get your point. I agree the flags passed in to the syscall should be used.
>
> - Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-04 20:31 [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC Lokesh Gidra
  2020-08-04 20:45 ` Eric Biggers
@ 2020-08-05  3:47 ` Aleksa Sarai
  2020-08-05  4:08   ` Eric Biggers
  1 sibling, 1 reply; 9+ messages in thread
From: Aleksa Sarai @ 2020-08-05  3:47 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: viro, stephen.smalley.work, casey, jmorris, kaleshsingh, dancol,
	surenb, linux-fsdevel, linux-kernel, nnk, jeffv, calin,
	kernel-team, yanfei.xu, syzbot+75867c44841cb6373570

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

On 2020-08-04, Lokesh Gidra <lokeshgidra@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-3997ab1d61ea@dancol.org/
> [2] https://lore.kernel.org/lkml/20200719165746.GJ2786714@ZenIV.linux.org.uk/
> 
> Fixes: d08ac70b1e0d (Wire UFFD up to SELinux)
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Reported-by: syzbot+75867c44841cb6373570@syzkaller.appspotmail.com
> Signed-off-by: Lokesh Gidra <lokeshgidra@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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-05  3:47 ` Aleksa Sarai
@ 2020-08-05  4:08   ` Eric Biggers
  2020-08-05  4:54     ` Lokesh Gidra
  2020-08-06 23:59     ` Aleksa Sarai
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Biggers @ 2020-08-05  4:08 UTC (permalink / raw)
  To: Aleksa Sarai
  Cc: Lokesh Gidra, viro, stephen.smalley.work, casey, jmorris,
	kaleshsingh, dancol, surenb, linux-fsdevel, linux-kernel, nnk,
	jeffv, calin, kernel-team, yanfei.xu,
	syzbot+75867c44841cb6373570

On Wed, Aug 05, 2020 at 01:47:58PM +1000, Aleksa Sarai wrote:
> On 2020-08-04, Lokesh Gidra <lokeshgidra@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).

Sure, O_CLOEXEC *should* be the default, but this is an existing syscall so it
has to keep the existing behavior.

> At the very least there should be a new flag added that sets O_CLOEXEC.

There already is one (but these patches broke it).

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-05  4:08   ` Eric Biggers
@ 2020-08-05  4:54     ` Lokesh Gidra
  2020-08-06 23:59     ` Aleksa Sarai
  1 sibling, 0 replies; 9+ messages in thread
From: Lokesh Gidra @ 2020-08-05  4:54 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Aleksa Sarai, Alexander Viro, Stephen Smalley, casey,
	James Morris, Kalesh Singh, Daniel Colascione,
	Suren Baghdasaryan, Linux FS Devel, linux-kernel, Nick Kralevich,
	Jeffrey Vander Stoep, Calin Juravle, kernel-team, yanfei.xu,
	syzbot+75867c44841cb6373570

On Tue, Aug 4, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Aug 05, 2020 at 01:47:58PM +1000, Aleksa Sarai wrote:
> > On 2020-08-04, Lokesh Gidra <lokeshgidra@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).
>
> Sure, O_CLOEXEC *should* be the default, but this is an existing syscall so it
> has to keep the existing behavior.
>
> > At the very least there should be a new flag added that sets O_CLOEXEC.
>
> There already is one (but these patches broke it).
>
I looked at the existing implementation, and the right thing is to
pass on the 'flags' (that is passed in to the syscall) to fetch 'fd'.

Besides, as you said in the other email thread,
anon_inode_getfile_secure() should be replaced with
anon_inode_getfd_secure(), which will remove this ambiguity.

I'll resend the patch series soon with all the changes that you proposed.
> - Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC
  2020-08-05  4:08   ` Eric Biggers
  2020-08-05  4:54     ` Lokesh Gidra
@ 2020-08-06 23:59     ` Aleksa Sarai
  1 sibling, 0 replies; 9+ messages in thread
From: Aleksa Sarai @ 2020-08-06 23:59 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Lokesh Gidra, viro, stephen.smalley.work, casey, jmorris,
	kaleshsingh, dancol, surenb, linux-fsdevel, linux-kernel, nnk,
	jeffv, calin, kernel-team, yanfei.xu,
	syzbot+75867c44841cb6373570

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

On 2020-08-04, Eric Biggers <ebiggers@kernel.org> wrote:
> On Wed, Aug 05, 2020 at 01:47:58PM +1000, Aleksa Sarai wrote:
> > On 2020-08-04, Lokesh Gidra <lokeshgidra@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).
> 
> Sure, O_CLOEXEC *should* be the default, but this is an existing syscall so it
> has to keep the existing behavior.

Ah, I missed that this was a UAPI breakage. :P

> > At the very least there should be a new flag added that sets O_CLOEXEC.
> 
> There already is one (but these patches broke it).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-08-06 23:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-04 20:31 [PATCH] Userfaultfd: Avoid double free of userfault_ctx and remove O_CLOEXEC Lokesh Gidra
2020-08-04 20:45 ` Eric Biggers
2020-08-04 20:49   ` Lokesh Gidra
2020-08-04 20:58     ` Eric Biggers
2020-08-04 23:34       ` Lokesh Gidra
2020-08-05  3:47 ` Aleksa Sarai
2020-08-05  4:08   ` Eric Biggers
2020-08-05  4:54     ` Lokesh Gidra
2020-08-06 23:59     ` Aleksa Sarai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).