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:

quoted

On Wed, Jun 17, 2020 at 03:03:24PM -0700, Kees Cook wrote:

quoted

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

quoted

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.

quoted

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