diff mbox series

[v5,4/7] pidfd: Replace open-coded partial fd_install_received()

Message ID 20200617220327.3731559-5-keescook@chromium.org
State New
Headers show
Series Add seccomp notifier ioctl that enables adding fds | expand

Commit Message

Kees Cook June 17, 2020, 10:03 p.m. UTC
The sock counting (sock_update_netprioidx() and sock_update_classid()) was
missing from pidfd's implementation of received fd installation. Replace
the open-coded version with a call to the new fd_install_received()
helper.

Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/pid.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

Comments

Christian Brauner July 6, 2020, 1:07 p.m. UTC | #1
On Wed, Jun 17, 2020 at 03:03:24PM -0700, Kees Cook wrote:
> The sock counting (sock_update_netprioidx() and sock_update_classid()) was
> missing from pidfd's implementation of received fd installation. Replace
> the open-coded version with a call to the new fd_install_received()
> helper.
> 
> Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/pid.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f1496b757162..24924ec5df0e 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -635,18 +635,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
>  	if (IS_ERR(file))
>  		return PTR_ERR(file);
>  
> -	ret = security_file_receive(file);
> -	if (ret) {
> -		fput(file);
> -		return ret;
> -	}
> -
> -	ret = get_unused_fd_flags(O_CLOEXEC);
> +	ret = fd_install_received(file, O_CLOEXEC);
>  	if (ret < 0)
>  		fput(file);
> -	else
> -		fd_install(ret, file);

So someone just sent a fix for pidfd_getfd() that was based on the
changes done here.

I've been on vacation so didn't have a change to review this series and
I see it's already in linux-next. This introduces a memory leak and
actually proves a point I tried to stress when adding this helper:
fd_install_received() in contrast to fd_install() does _not_ consume a
reference because it takes one before it calls into fd_install(). That
means, you need an unconditional fput() here both in the failure and
error path.
I strongly suggest though that we simply align the behavior between
fd_install() and fd_install_received() and have the latter simply
consume a reference when it succeeds! Imho, this bug proves that I was
right to insist on this before. ;)

Thanks!
Christian
Kees Cook July 6, 2020, 3:34 p.m. UTC | #2
On Mon, Jul 06, 2020 at 03:07:13PM +0200, Christian Brauner wrote:
> On Wed, Jun 17, 2020 at 03:03:24PM -0700, Kees Cook wrote:
> > The sock counting (sock_update_netprioidx() and sock_update_classid()) was
> > missing from pidfd's implementation of received fd installation. Replace
> > the open-coded version with a call to the new fd_install_received()
> > helper.
> > 
> > Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  kernel/pid.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> > 
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index f1496b757162..24924ec5df0e 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -635,18 +635,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
> >  	if (IS_ERR(file))
> >  		return PTR_ERR(file);
> >  
> > -	ret = security_file_receive(file);
> > -	if (ret) {
> > -		fput(file);
> > -		return ret;
> > -	}
> > -
> > -	ret = get_unused_fd_flags(O_CLOEXEC);
> > +	ret = fd_install_received(file, O_CLOEXEC);
> >  	if (ret < 0)
> >  		fput(file);
> > -	else
> > -		fd_install(ret, file);
> 
> So someone just sent a fix for pidfd_getfd() that was based on the
> changes done here.

Hi! Ah yes, that didn't get CCed to me. I'll go reply.

> I've been on vacation so didn't have a change to review this series and
> I see it's already in linux-next. This introduces a memory leak and
> actually proves a point I tried to stress when adding this helper:
> fd_install_received() in contrast to fd_install() does _not_ consume a
> reference because it takes one before it calls into fd_install(). That
> means, you need an unconditional fput() here both in the failure and
> error path.

Yup, this was a mistake in my refactoring of the pidfs changes.

> I strongly suggest though that we simply align the behavior between
> fd_install() and fd_install_received() and have the latter simply
> consume a reference when it succeeds! Imho, this bug proves that I was
> right to insist on this before. ;)

I still don't agree: it radically complicates the SCM_RIGHTS and seccomp
cases. The primary difference is that fd_install() cannot fail, and it
was optimized for this situation. The other file-related helpers that
can fail do not consume the reference, so this is in keeping with those
as well.
Christian Brauner July 6, 2020, 4:12 p.m. UTC | #3
On Mon, Jul 06, 2020 at 08:34:06AM -0700, Kees Cook wrote:
> On Mon, Jul 06, 2020 at 03:07:13PM +0200, Christian Brauner wrote:
> > On Wed, Jun 17, 2020 at 03:03:24PM -0700, Kees Cook wrote:
> > > The sock counting (sock_update_netprioidx() and sock_update_classid()) was
> > > missing from pidfd's implementation of received fd installation. Replace
> > > the open-coded version with a call to the new fd_install_received()
> > > helper.
> > > 
> > > Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > >  kernel/pid.c | 11 +----------
> > >  1 file changed, 1 insertion(+), 10 deletions(-)
> > > 
> > > diff --git a/kernel/pid.c b/kernel/pid.c
> > > index f1496b757162..24924ec5df0e 100644
> > > --- a/kernel/pid.c
> > > +++ b/kernel/pid.c
> > > @@ -635,18 +635,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
> > >  	if (IS_ERR(file))
> > >  		return PTR_ERR(file);
> > >  
> > > -	ret = security_file_receive(file);
> > > -	if (ret) {
> > > -		fput(file);
> > > -		return ret;
> > > -	}
> > > -
> > > -	ret = get_unused_fd_flags(O_CLOEXEC);
> > > +	ret = fd_install_received(file, O_CLOEXEC);
> > >  	if (ret < 0)
> > >  		fput(file);
> > > -	else
> > > -		fd_install(ret, file);
> > 
> > So someone just sent a fix for pidfd_getfd() that was based on the
> > changes done here.
> 
> Hi! Ah yes, that didn't get CCed to me. I'll go reply.
> 
> > I've been on vacation so didn't have a change to review this series and
> > I see it's already in linux-next. This introduces a memory leak and
> > actually proves a point I tried to stress when adding this helper:
> > fd_install_received() in contrast to fd_install() does _not_ consume a
> > reference because it takes one before it calls into fd_install(). That
> > means, you need an unconditional fput() here both in the failure and
> > error path.
> 
> Yup, this was a mistake in my refactoring of the pidfs changes.

I already did.

> 
> > I strongly suggest though that we simply align the behavior between
> > fd_install() and fd_install_received() and have the latter simply
> > consume a reference when it succeeds! Imho, this bug proves that I was
> > right to insist on this before. ;)
> 
> I still don't agree: it radically complicates the SCM_RIGHTS and seccomp

I'm sorry, I don't buy it yet, though I might've missed something in the
discussions: :)
After applying the patches in your series this literally is just (which
is hardly radical ;):

diff --git a/fs/file.c b/fs/file.c
index 9568bcfd1f44..26930b2ea39d 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -974,7 +974,7 @@ int __fd_install_received(int fd, struct file *file, int __user *ufd,
        }

        if (fd < 0)
-               fd_install(new_fd, get_file(file));
+               fd_install(new_fd, file);
        else {
                new_fd = fd;
                error = replace_fd(new_fd, file, o_flags);
diff --git a/net/compat.c b/net/compat.c
index 71494337cca7..605a5a67200c 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -298,9 +298,11 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
        int err = 0, i;

        for (i = 0; i < fdmax; i++) {
-               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
-               if (err < 0)
+               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
+               if (err < 0) {
+                       fput(scm->fp->fp[i]);
                        break;
+               }
        }

        if (i > 0) {
diff --git a/net/core/scm.c b/net/core/scm.c
index b9a0442ebd26..0d06446ae598 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -306,9 +306,11 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
        }

        for (i = 0; i < fdmax; i++) {
-               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
-               if (err < 0)
+               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
+               if (err < 0) {
+                       fput(scm->fp->fp[i]);
                        break;
+               }
        }

        if (i > 0) {

> cases. The primary difference is that fd_install() cannot fail, and it
> was optimized for this situation. The other file-related helpers that
> can fail do not consume the reference, so this is in keeping with those
> as well.

That's not a real problem. Any function that can fail and which consumes
a reference on success is assumed to not mutate the reference if it
fails anywhere. So I don't see that as an issue.

The problem here is that the current patch invites bugs and has already
produced one because fd_install() and fd_install_*() have the same
naming scheme but different behavior when dealing with references.
That's just not a good idea.

Christian
Christian Brauner July 6, 2020, 4:38 p.m. UTC | #4
On Mon, Jul 06, 2020 at 06:12:47PM +0200, Christian Brauner wrote:
> On Mon, Jul 06, 2020 at 08:34:06AM -0700, Kees Cook wrote:
> > On Mon, Jul 06, 2020 at 03:07:13PM +0200, Christian Brauner wrote:
> > > On Wed, Jun 17, 2020 at 03:03:24PM -0700, Kees Cook wrote:
> > > > The sock counting (sock_update_netprioidx() and sock_update_classid()) was
> > > > missing from pidfd's implementation of received fd installation. Replace
> > > > the open-coded version with a call to the new fd_install_received()
> > > > helper.
> > > > 
> > > > Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > > >  kernel/pid.c | 11 +----------
> > > >  1 file changed, 1 insertion(+), 10 deletions(-)
> > > > 
> > > > diff --git a/kernel/pid.c b/kernel/pid.c
> > > > index f1496b757162..24924ec5df0e 100644
> > > > --- a/kernel/pid.c
> > > > +++ b/kernel/pid.c
> > > > @@ -635,18 +635,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
> > > >  	if (IS_ERR(file))
> > > >  		return PTR_ERR(file);
> > > >  
> > > > -	ret = security_file_receive(file);
> > > > -	if (ret) {
> > > > -		fput(file);
> > > > -		return ret;
> > > > -	}
> > > > -
> > > > -	ret = get_unused_fd_flags(O_CLOEXEC);
> > > > +	ret = fd_install_received(file, O_CLOEXEC);
> > > >  	if (ret < 0)
> > > >  		fput(file);
> > > > -	else
> > > > -		fd_install(ret, file);
> > > 
> > > So someone just sent a fix for pidfd_getfd() that was based on the
> > > changes done here.
> > 
> > Hi! Ah yes, that didn't get CCed to me. I'll go reply.
> > 
> > > I've been on vacation so didn't have a change to review this series and
> > > I see it's already in linux-next. This introduces a memory leak and
> > > actually proves a point I tried to stress when adding this helper:
> > > fd_install_received() in contrast to fd_install() does _not_ consume a
> > > reference because it takes one before it calls into fd_install(). That
> > > means, you need an unconditional fput() here both in the failure and
> > > error path.
> > 
> > Yup, this was a mistake in my refactoring of the pidfs changes.
> 
> I already did.
> 
> > 
> > > I strongly suggest though that we simply align the behavior between
> > > fd_install() and fd_install_received() and have the latter simply
> > > consume a reference when it succeeds! Imho, this bug proves that I was
> > > right to insist on this before. ;)
> > 
> > I still don't agree: it radically complicates the SCM_RIGHTS and seccomp
> 
> I'm sorry, I don't buy it yet, though I might've missed something in the
> discussions: :)
> After applying the patches in your series this literally is just (which
> is hardly radical ;):
> 
> diff --git a/fs/file.c b/fs/file.c
> index 9568bcfd1f44..26930b2ea39d 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -974,7 +974,7 @@ int __fd_install_received(int fd, struct file *file, int __user *ufd,
>         }
> 
>         if (fd < 0)
> -               fd_install(new_fd, get_file(file));
> +               fd_install(new_fd, file);
>         else {
>                 new_fd = fd;
>                 error = replace_fd(new_fd, file, o_flags);
> diff --git a/net/compat.c b/net/compat.c
> index 71494337cca7..605a5a67200c 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -298,9 +298,11 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
>         int err = 0, i;
> 
>         for (i = 0; i < fdmax; i++) {
> -               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> -               if (err < 0)
> +               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
> +               if (err < 0) {
> +                       fput(scm->fp->fp[i]);
>                         break;
> +               }
>         }
> 
>         if (i > 0) {
> diff --git a/net/core/scm.c b/net/core/scm.c
> index b9a0442ebd26..0d06446ae598 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -306,9 +306,11 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>         }
> 
>         for (i = 0; i < fdmax; i++) {
> -               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> -               if (err < 0)
> +               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
> +               if (err < 0) {
> +                       fput(scm->fp->fp[i]);
>                         break;
> +               }
>         }
> 
>         if (i > 0) {
> 
> > cases. The primary difference is that fd_install() cannot fail, and it
> > was optimized for this situation. The other file-related helpers that
> > can fail do not consume the reference, so this is in keeping with those
> > as well.
> 
> That's not a real problem. Any function that can fail and which consumes
> a reference on success is assumed to not mutate the reference if it
> fails anywhere. So I don't see that as an issue.
> 
> The problem here is that the current patch invites bugs and has already
> produced one because fd_install() and fd_install_*() have the same
> naming scheme but different behavior when dealing with references.
> That's just not a good idea.

That being said, if you and others feel that this worry is nonsense then
sure let's fix the bug that this introduces in this series and move on.
If you do are you going to resend?

Christian
Kees Cook July 6, 2020, 7:30 p.m. UTC | #5
On Mon, Jul 06, 2020 at 06:12:45PM +0200, Christian Brauner wrote:
> On Mon, Jul 06, 2020 at 08:34:06AM -0700, Kees Cook wrote:
> > Yup, this was a mistake in my refactoring of the pidfs changes.
> 
> I already did.

Er, what? (I had a typo in my quote: s/pidfs/pidfd/.) I was trying to
say that this was just a mistake in my refactoring of the pidfd usage of
the new helper.

> > I still don't agree: it radically complicates the SCM_RIGHTS and seccomp
> 
> I'm sorry, I don't buy it yet, though I might've missed something in the
> discussions: :)
> After applying the patches in your series this literally is just (which
> is hardly radical ;):

Agreed, "radical" was too strong.

> diff --git a/fs/file.c b/fs/file.c
> index 9568bcfd1f44..26930b2ea39d 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -974,7 +974,7 @@ int __fd_install_received(int fd, struct file *file, int __user *ufd,
>         }
> 
>         if (fd < 0)
> -               fd_install(new_fd, get_file(file));
> +               fd_install(new_fd, file);
>         else {
>                 new_fd = fd;
>                 error = replace_fd(new_fd, file, o_flags);
> diff --git a/net/compat.c b/net/compat.c
> index 71494337cca7..605a5a67200c 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -298,9 +298,11 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
>         int err = 0, i;
> 
>         for (i = 0; i < fdmax; i++) {
> -               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> -               if (err < 0)
> +               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
> +               if (err < 0) {
> +                       fput(scm->fp->fp[i]);
>                         break;
> +               }
>         }
> 
>         if (i > 0) {
> diff --git a/net/core/scm.c b/net/core/scm.c
> index b9a0442ebd26..0d06446ae598 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -306,9 +306,11 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>         }
> 
>         for (i = 0; i < fdmax; i++) {
> -               err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> -               if (err < 0)
> +               err = fd_install_received_user(get_file(scm->fp->fp[i]), cmsg_data + i, o_flags);
> +               if (err < 0) {
> +                       fput(scm->fp->fp[i]);
>                         break;
> +               }
>         }
> 
>         if (i > 0) {

But my point stands: I really dislike this; suddenly the caller needs to
manage this when it should be an entirely internal detail to the
function. It was only pidfd doing it wrong, and that was entirely my
fault in the conversion.

> The problem here is that the current patch invites bugs and has already
> produced one because fd_install() and fd_install_*() have the same
> naming scheme but different behavior when dealing with references.
> That's just not a good idea.

I will rename the helper and add explicit documentation, but I really
don't think callers should have to deal with managing the helper's split
ref lifetime.
diff mbox series

Patch

diff --git a/kernel/pid.c b/kernel/pid.c
index f1496b757162..24924ec5df0e 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -635,18 +635,9 @@  static int pidfd_getfd(struct pid *pid, int fd)
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
-	ret = security_file_receive(file);
-	if (ret) {
-		fput(file);
-		return ret;
-	}
-
-	ret = get_unused_fd_flags(O_CLOEXEC);
+	ret = fd_install_received(file, O_CLOEXEC);
 	if (ret < 0)
 		fput(file);
-	else
-		fd_install(ret, file);
-
 	return ret;
 }